Repository navigation
handle micrometer interval vs metrics interval - #2801
jackshirazi merged 8 commits into
Conversation
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
💚 Flaky test reportTests succeeded. 🤖 GitHub commentsExpand to view the GitHub comments
To re-run your PR in the CI, just comment with:
|
|
|
||
| //interval is actually this plus a random amount of ms more due to processing delays | ||
| //this means that an occasional step metric will be skipped. This is deemed acceptable | ||
| private static final long INTERVAL_BETWEEN_CHECKS_IN_MILLISECONDS = 1000L; |
There was a problem hiding this comment.
Steps have to be a multiple of 1s. I wonder if we could avoid this limitation and potentially also simplify this reporter by having one scheduled job per MeterRegistry or per distinct step duration.
There was a problem hiding this comment.
I'm trying to figure if we'd be changing the user-contract - I've set the metrics-interval to 1 second resolution because the alternative of any value (with 1 second minimum) seems to me likely to cause problems. But I haven't though it through. There was a a lot of testing that got me to here, with a lot of different implementations and it still has intermittent failures, so I'm wondering whether this implementation works for now and we can revisit later, or whether I should be retrying with your suggestion now
There was a problem hiding this comment.
Wasn't the previous user contract that step_duration < metric_interval just wouldn't work correctly because it would always skip some steps? So this new restrictions seems fine to me.
To be honest, this all seems quiet complicated without ever having a 100% solution.
Imo, the most maintainable solution would be to request users to always use CUMULATIVE due to the micrometer implementation restrictions. This shouldn't be too hard for users (I think?) due to the fact that we request a setup of a SimpleMeterRegistry anyway. We could then allow a configuration of which Meters are exposed cumulative vs step on our side (e.g. step_metrics with wildcard matchers like disable_metrics) and do the delta calculations safely by ourselves.
I'm not a micrometer expert, so take this all with a big grain of salt and feel free to correct me!
There was a problem hiding this comment.
A lot of applications already have micrometer meters registered independent of our (or any) agent, and they'll use the stats in their own ways, we're just piggybacking off what's in there already. So I don't think it's valid to insist on CUMULATIVE only.
Absolutely agree this is complicated and not ideal
There was a problem hiding this comment.
Within TSDB, they're planning to create a rate aggregation that can deal with monotonically incrementing counters much better. Maybe it's good enough to document that we recommend STEP/delta for now. In the future, CUMULATIVE, might be a perfectly valid option, too.
| return ((StepRegistryConfig) (meterRegistry.config())).step().toMillis(); | ||
| } | ||
| if (meterRegistry instanceof SimpleMeterRegistry) { | ||
| SimpleConfig config = configMap.get(meterRegistry); |
There was a problem hiding this comment.
Our current documentation states the following:
Attach the agent, and you’re done! The agent automatically detects all MeterRegistry instances and reports all metrics to APM Server (in addition to where they originally report). When attaching the agent after the application has already started, the agent detects a MeterRegistry when calling any public method on it.
This is not 100% true after this change, as the behaviour can differ between attachment on startup vs later.
If attaching after startup, we might miss the construction of the SimpleMeterRegistry here.
Is this known already?
An alternative would be to instrument some other side-effect free method on SimpleMeterRegistry and to call it ourselves, but this also isn't guaranteed to work for subclasses of SimpleMeterRegistry unless we directly call the method via MethodHandles.
This might be too much effort and we might as well just add this limitation to the docs instead.
There was a problem hiding this comment.
excellent point. I'll revisit how we get the config
There was a problem hiding this comment.
I've added support for attaching after startup. Unfortunately you can't redefine methods after they've been loaded, and there is only one public method in SimpleMeterRegistry to use to trigger getting the private config, and that is only available from 1.9.0 of micrometer, so the post-attach mechanism only works in that scenario. Which I think is fine
|
|
||
| //interval is actually this plus a random amount of ms more due to processing delays | ||
| //this means that an occasional step metric will be skipped. This is deemed acceptable | ||
| private static final long INTERVAL_BETWEEN_CHECKS_IN_MILLISECONDS = 1000L; |
There was a problem hiding this comment.
Wasn't the previous user contract that step_duration < metric_interval just wouldn't work correctly because it would always skip some steps? So this new restrictions seems fine to me.
To be honest, this all seems quiet complicated without ever having a 100% solution.
Imo, the most maintainable solution would be to request users to always use CUMULATIVE due to the micrometer implementation restrictions. This shouldn't be too hard for users (I think?) due to the fact that we request a setup of a SimpleMeterRegistry anyway. We could then allow a configuration of which Meters are exposed cumulative vs step on our side (e.g. step_metrics with wildcard matchers like disable_metrics) and do the delta calculations safely by ourselves.
I'm not a micrometer expert, so take this all with a big grain of salt and feel free to correct me!
…ub.com/jackshirazi/apm-agent-java into micrometer-handle-misaligned-intervals
What does this PR do?
Fix the micrometer plugin so that it correctly handles intervals
Checklist