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

Async Metrics #4661

Merged
merged 9 commits into from
Mar 23, 2020
Merged

Async Metrics #4661

merged 9 commits into from
Mar 23, 2020

Conversation

felixdesouza
Copy link
Contributor

Goals (and why):
As part of some of the conjure-undertow changes, async versions of requests were added. The sad part is, the metrics as we know currently are off since they measure the time it takes to be given back the ListenableFuture vs how long the ListenableFuture takes to complete.

Ideally this would go in https://github.com/palantir/tritium, but given that we've hacked together the TaggedMetricsInvocationHandler for our own (nefarious?) gains, I didn't want to unwind that change as I'm working on Timelock Partitioning etc. @schlosna for SA. My proposed impl would be that it goes in ByteBuddyInstrumentationAdvice or InvocationEventProxy then every InvocationHandler gets async metrics stuff for free!

Implementation Description (bullets):

  • Backport some of the metrics changes in the TaggedMetricsInvocationHandler wrt metric name cache etc. Since the metric name doesn't change for most invocations we're wasting time doing the string concat and the allocations for MetricName. For the ones with the extra tags, have a specialised cache for those calls.
  • Check if the return type is ListenableFuture and if so, add a listener that will update the metric. Otherwise we leave it as is.

Testing (What was existing testing like? What have you done to improve it?):

  • Hard to test properly since we can't change the future that gets returned, we can only act on it => we can have a race condition where if we were to wait for the future to be done, it's not necessarily the case that the metrics would have been tracked correctly. Only fix I see for this is in upstream in tritium.

Concerns (what feedback would you like?):

Where should we start reviewing?:

  • TaggedMetricsEventInvocationHandler
  • AtlasDbMetrics

Priority (whenever / two weeks / yesterday):
ASAP! Some metrics in our internal dashboard is a lie until fixed.

@changelog-app
Copy link

changelog-app bot commented Mar 23, 2020

Generate changelog in changelog/@unreleased

Type

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

Description

Methods returning ListenableFutures now have their metrics accurately tracked.

Check the box to generate changelog(s)

  • Generate changelog entry

Copy link
Contributor

@Jolyon-S Jolyon-S left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the semantics are good, just a few stylistic comments.

Copy link
Contributor

@Jolyon-S Jolyon-S left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

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

Successfully merging this pull request may close these issues.

None yet

2 participants