Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider adding send-like method to MetricBuilder that ignores results #65

Closed
56quarters opened this issue Mar 15, 2018 · 5 comments
Closed

Comments

@56quarters
Copy link
Owner

Per #63, an attribute was added to the MetricBuilder to warn when it was unused. Thus, code like the following (incorrect, since the metric isn't sent) would generate a warning:

client.count_with_tags("some.key", 4)
    .with_tag("region", "us-east-1")
    .with_tag("host", "container-12345-abc");

However, the following (totally reasonable) code also generates a warning:

client.count_with_tags("some.key", 4)
    .with_tag("region", "us-east-1")
    .with_tag("host", "container-12345-abc")
    .send();

To avoid the warning, users need to do something with the Result from .send(), like...

client.count_with_tags("some.key", 4)
    .with_tag("region", "us-east-1")
    .with_tag("host", "container-12345-abc")
    .send()
    .ok();

To solve this, @robinst has suggested adding another method to MetricBuilder that sends the metric but does not return a result so that callers aren't forced to do something perfunctory with it.

Example (using the name send_quiet which I'm not at all attached to):

client.count_with_tags("some.key", 4)
    .with_tag("region", "us-east-1")
    .with_tag("host", "container-12345-abc")
    .send_quiet();

What are people's thoughts on this? Does adding another send-like method seem acceptable? If so, any thoughts on a name?

/cc @robinst @pjenvey

@robinst
Copy link
Contributor

robinst commented Mar 15, 2018

I was wondering if there's an example of a naming convention for that in the standard library. The only similar thing I found so far is _unchecked: https://doc.rust-lang.org/std/primitive.str.html?search=unchecked

But those methods are marked unsafe because the result might be bad, so it's not really the same thing.

Another option might be send_ignore()? Just playing around with names.

@56quarters
Copy link
Owner Author

send_ignore seems like a good choice to me.

@56quarters 56quarters added this to the 0.13.3 milestone Mar 26, 2018
@pjenvey
Copy link
Contributor

pjenvey commented Mar 26, 2018

I think every rust metrics lib I've seen returns a Result. I'm wondering if it's the wrong default in the first place w/ the nature of metrics, do most users ignore send results?

Maybe StatsdClient could have an error_handler (say defaulting to logging an error!) while changing send to not return a Result, w/ an alternative send API that does. Then at least the typical behavior isn't a complete ignore

@robinst
Copy link
Contributor

robinst commented Mar 27, 2018

Yeah that would also work for us. So you'd probably have send() -> () and try_send() -> Result methods?

@56quarters
Copy link
Owner Author

56quarters commented Mar 27, 2018

I like the idea of some sort of error handler such that errors aren't completely ignored. However, I'd rather not change the existing send method to send() -> () and instead add a new method that makes use of the error handler. Actually, I guess the breaking change might be fine since it's limited to the MetricBuilder. Thoughts?

@56quarters 56quarters modified the milestones: 0.13.3, 0.14.0 Mar 28, 2018
56quarters added a commit that referenced this issue Mar 29, 2018
Change the behavior of the `MetricBuilder::send()` method to not return
a result based on the sending of the metric. Users that wan the result
can now use the `MetricBuilder::try_send()` method for sending
metrics.

On success, the output from sending the metric with `.send()` is
discarded. On error, a user supplied error handler is invoked to consume
the error. By default, if no handler is supplied, the errors are
sillently discarded.

The motivation for discarding errors has a few parts:
* We don't want to add another dependency to Cadence.
* There have been issues opened in the past to remove logging from
  Cadence.
* Users that want to do something more elaborate will have a better
  idea of the right course of action that us.

Fixes #65
56quarters added a commit that referenced this issue Mar 30, 2018
Change the behavior of the `MetricBuilder::send()` method to not return
a result based on the sending of the metric. Users that want the result
can now use the `MetricBuilder::try_send()` method for sending
metrics.

On success, the output from sending the metric with `.send()` is
discarded. On error, a user supplied error handler is invoked to consume
the error. By default, if no handler is supplied, the errors are
silently discarded.

The motivation for discarding errors has a few parts:
* We don't want to add another dependency to Cadence.
* There have been issues opened in the past to remove logging from
  Cadence.
* Users that want to do something more elaborate will have a better
  idea of the right course of action that us.

Fixes #65
56quarters added a commit that referenced this issue Mar 30, 2018
Change the behavior of the `MetricBuilder::send()` method to not return
a result based on the sending of the metric. Users that want the result
can now use the `MetricBuilder::try_send()` method for sending
metrics.

On success, the output from sending the metric with `.send()` is
discarded. On error, a user supplied error handler is invoked to consume
the error. By default, if no handler is supplied, the errors are
silently discarded.

The motivation for discarding errors has a few parts:
* We don't want to add another dependency to Cadence.
* There have been issues opened in the past to remove logging from
  Cadence.
* Users that want to do something more elaborate will have a better
  idea of the right course of action than us.

Fixes #65
56quarters added a commit that referenced this issue Apr 6, 2018
Change the behavior of the `MetricBuilder::send()` method to not return
a result based on the sending of the metric. Users that want the result
can now use the `MetricBuilder::try_send()` method for sending
metrics.

On success, the output from sending the metric with `.send()` is
discarded. On error, a user supplied error handler is invoked to consume
the error. By default, if no handler is supplied, the errors are
silently discarded.

The motivation for discarding errors has a few parts:
* We don't want to add another dependency to Cadence.
* There have been issues opened in the past to remove logging from
  Cadence.
* Users that want to do something more elaborate will have a better
  idea of the right course of action than us.

Fixes #65
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants