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

Add hook for statsd integration #7417

Merged
merged 4 commits into from
Feb 8, 2021
Merged

Conversation

5antelope
Copy link
Member

@5antelope 5antelope commented Jan 31, 2021

Signed-off-by: crowu y.wu4515@gmail.com

Description

This PR adds a hook interface for different backend to register timing and histogram metrics.

The main motivation comes from the integration with statsd:
the statsd API for timing / histogram takes a name and value, and it will aggregate the percentile metrics for us. This makes it really hard to parse the histogram data from vitess as latency in statsd.
We currently just represent them as a plain gauges with bucket suffix - this is not a very helpful metrics to understand the latency performance.

This PR adds a statsdHook, different stats backend can choose to add different hooks for timing and histogram, and when we add timing / histogram, Vitess will also try to invoke the hook function as well.

One caveat: this setup only applies to stats that published after the register functions in servenv.OnRun being called. For metrics that published before vitess server starts, we will just ignore them since we haven't register the hook yet

Although we are plan to integrate with OpenTelemetry , I feel there is still value to add this support to unblock any existing statsd use case.

Related Issue(s)

Checklist

  • Should this PR be backported?
  • Tests were added or are not required
  • Documentation was added or is not required

Deployment Notes

NA

Impacted Areas in Vitess

Components that this PR will affect:

  • Query Serving - mostly for stats reporting
  • VReplication
  • Cluster Management
  • Build/CI
  • VTAdmin

Signed-off-by: crowu <y.wu4515@gmail.com>
@5antelope
Copy link
Member Author

I don't think the failing test is caused by the change... is there a way to manually trigger the CI builds again? 🤔

@5antelope
Copy link
Member Author

Looks like all tests are passed after reran them!
Thanks @deepthi for helping :-)

Signed-off-by: crowu <y.wu4515@gmail.com>
Signed-off-by: crowu <y.wu4515@gmail.com>
Signed-off-by: crowu <y.wu4515@gmail.com>
Copy link
Contributor

@sougou sougou left a comment

Choose a reason for hiding this comment

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

Sorry about the delayed review. This looks good. Some tests didn't complete, and no way to restart. But they don't look related. I'll just force-merge.

@sougou sougou merged commit 676911a into vitessio:master Feb 8, 2021
@5antelope 5antelope deleted the ywu/stats branch February 10, 2021 00:20
@askdba askdba added this to the v10.0 milestone Feb 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants