SignalFx reporter is not converting durations according to reporter/metric config about signalfx-java HOT 12 CLOSED

@mpetazzoni Sad to hear the decision but thanks for detailed response.
I would also encourage you guys to make it clear in SignalFx Reporter description that implementation does not support such conversion (which contradicts SheduledReporter superclass) and remove/deprecate builder methods that don't do anything but confuse library user:

        public Builder setRateUnit(TimeUnit rateUnit) {
            this.rateUnit = rateUnit;
            return this;
        }

        public Builder setDurationUnit(TimeUnit durationUnit) {
            this.durationUnit = durationUnit;
            return this;
        }

from signalfx-java.

@mpetazzoni @beccatortell I'd say this ticket can be closed because with introduction of micrometer and support of y-axes units in SFX this is not relevant anymore

from signalfx-java.

I also found PullRequest which attempt to address this
14c0495
But was not merged. I understand that suggested solution was not ideal (overriding duration units to ALL metrics, instead of using PerMetric duration units).
Is there a plan to address this? Seems to be handled correctly in other reporters:

https://github.com/dropwizard/metrics/blob/3.2-development/metrics-graphite/src/main/java/com/codahale/metrics/graphite/GraphiteReporter.java#L286

https://github.com/dropwizard/metrics/blob/3.2-development/metrics-core/src/main/java/com/codahale/metrics/Slf4jReporter.java#L241

from signalfx-java.

@M1strall we are considering the best way to handle this, but I was wondering what you meant by PerMetric duration units? As best I can tell in reading the Codahale source, you specify the durationUnit as a parameter when building the reporter instance and it is supposed to convert all timer metrics to that scale before reporting. Timer metrics themselves appear to be left in nanoseconds when created, and don't carry around information about unit/scale. Are you aware of some interface for specifying units per metric with the reporters?

Do you want all of your duration metrics to be in millisecond resolution or are you trying to achieve some kind of mix?

from signalfx-java.

Sorry, I meant Per reporter duration/rateUnit configuration, the same thing you are referring to.
We will be using Graphite & SignalFx side by side for some time, so for now my goal is keep same behavior between Graphite + MetricsProxy reporting and SignalFx direct reporting, when we can specify conversion units via reporter config.
Current workaround is to adjust all our dashboards and use "Scale" aggregation when data comes from SignalFx reporter.

from signalfx-java.

@M1strall About PR #32, what did you think of my comment #32 (comment) ? My main concern is that you can end up with multiple timeseries under the same metric that end up having different values because some are emitted with a value converted to milliseconds (for example), while others are not converted and are in nanoseconds.

Because of this, I feel like it's much cleaner, and safer, to not perform any modification of the measured data at its point of origin (no unit conversion) and let SignalFx do that, in the places you actually need (for presentation / human readability purposes).

from signalfx-java.

@mpetazzoni I'm not sure if I understand your concern correctly. So here is my vision:

1. Metric library always records metrics in Nanoseconds, which makes a lot of sense and ensures they are collected in the uniform way across all metric sets.
2. Metric Reporter (in our case we have 2 different Graphite reporter and SignalFx reporter) is a presentation layer. For user readability and user convenience purposes reporter can convert metric value to desired format. What format to pick is up to user of reporter. No change should be applied to original metric values so I'm not sure what is the concern.
3. SignalFx reporter currently ignores user configuration and this is the issue.

I think if you leave default behavior as it today - it's also fine, but it would be great to give user an option to configure reporter according to it's interface.

@zeldigas If you have good example that would be great

from signalfx-java.

Example is pretty simple: there is no intersecting metrics across our applications as we mainly focused on using time metrics on controllers level and all app instances have centralized config so that there is no concerns that some parts of reporters would use one "scale" and other would use another one.
From usage perspective it's far easier to have internal agreement that application uses milliseconds, rather than always keep in mind what is the unit when setting up charts.

I don't really get what is the problem with giving users "ability" with defaults of your choise (as the owners of the library). Following this way you can set defaults to nanos, but still leaving users an options to decide.

P.S. I know that we can just fork your repo and publish customized copy of reporter, but I don't get why doing this split in such a simple case

from signalfx-java.

I disagree with your statement that the metric reporter is a presentation layer. The presentation layer is Graphite's UI, or SignalFx's UI, presenting you the data. If the data reaches your monitoring system (Graphite or SignalFx or other) already modified at the source, it's "too late".

My point is that yes, the metric library should always measure in nanosecond. But that data should also be sent to, and recorded by, your monitoring system, in nanosecond. It's then up to the user to make the presentation layer (aka, the graph) display the data at the level of precision that they want for better readability. Whether that's tedious or not is a UI/UX concern that we can address differently and that we are independently working on.

But regardless of those points above, there is a bigger problem with what you suggest, and that's a problem inherent to the Codahale reporter API in that it lets people do this conversion at all. And this particular problem is why we chose not to support that conversion feature in our reporter. Imagine that you have multiple instances of a service that handles requests, and you are measuring with a Timer how long those requests take. You haven't configured a conversion, so the timing metrics from that timer (99th, median, mean, etc) are reported in nanoseconds. All good so far.

Now, you introduce a change to this service's code that sets up unit conversion to milliseconds on the Codahale reporter, and you deploy this new build to your canary instance. Now you have one instance that reports data in milliseconds, and the others that still report in nanoseconds. It is now impossible for you to look at your data across this service tier accurately, at least not until you have fully deployed this new change. And even after you've done so, data before the change and after the change can no longer be compared easily. Notice that no "centralized config" or "no intersecting metrics" here solve this, because this is something that's in code that gets deployed, specified when the reporter is instantiated, and that is not dynamically configurable.

This is why it's always a better idea to record, report, and store, in the absolute base unit without conversion (here, nanoseconds), and convert at the presentation layer. Maybe Alice likes to see things in milliseconds, but Bob really really wants to see nanosecond precision, while Chelsea prefers seconds!

We believe this is the right thing to do and a best practice to follow to make sure that you don't fall into those situations. In my opinion, the reason this convertDuration exists at all in Codahale is because other monitoring systems like Graphite have very poor UI/UX to make those presentation-time conversions easy.

from signalfx-java.

Your arguments could be valid for a number of cases, not for all. With "nanoseconds" approach you force your users to alwasy use "scale" function because no good man would like to work with nanosecons scale by default knowing that apps is operating with "milliseconds" or second values, especially keeping in mind that SignalFx does not support such simple and uber usefull feature like "y axes type" compared to Grafana. From UX standpoint that's very doubtful approach.

from signalfx-java.

@zeldigas We don't have Y axis type indeed, not yet, but we have an open feature request that will get worked on soon. I can't make promises on roadmap obviously, but remember that SignalFx uses SignalFx itself, and this is something we want too! :)

from signalfx-java.

SignalFx Product Management here. Thanks @M1strall and @zeldigas for your request, and everyone for engaging in the resulting discussion. We agree that the SignalFx reporter should respect the user's choice to convert durations to the desired timescale, and will implement this feature.

We also think that unit conversion should be primarily handled in charts, and definitely understand that this isn't as easy as it could be. As @mpetazzoni mentioned we've requested features to make it easier to display the desired units. In future, hopefully the ability to do this in UI will allow you to sidestep the potential harm that @mpetazzoni described in his response.

Thanks again for the request and we'll keep you updated.

from signalfx-java.