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

feat(observability)(WIP): allow custom latency distribution buckets #7646

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

jfreeland
Copy link

Description

Work In Progress.

This is intended to address #7641 and add support for users to provide an alternative list of distributions thresholds.

No tests. Not entirely sure this is the best approach. Not validated at all yet.

Issue reference

Please reference the issue this PR will close: #[issue number]

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

@yaron2
Copy link
Member

yaron2 commented Apr 10, 2024

@jfreeland are you still working on this?

@jfreeland
Copy link
Author

@jfreeland are you still working on this?

my wedding, honeymoon, and day job have gotten in the way. i won't have time to revisit and test for the next couple weeks, unfortunately. if someone else has time and wants to make it happen elsewhere that'd be awesome. if not i'll revisit as soon as i can.

@jfreeland jfreeland changed the title feat(observability): allow custom latency distribution buckets feat(observability)(WIP): allow custom latency distribution buckets May 20, 2024
@jfreeland jfreeland marked this pull request as ready for review May 20, 2024 00:27
@jfreeland jfreeland requested review from a team as code owners May 20, 2024 00:27
@jfreeland
Copy link
Author

jfreeland commented May 20, 2024

I think there's still work to do here but I think this is 'closer'. I didn't get a chance to dig around the Makefile and reverse engineer the quickest path to building my own containers and pushing to my own registry so that I could test this out in my development environment.

I seem to recall a couple months ago that I regenerated the generated files myself but I didn't do that tonight. I don't recall seeing that I needed to in CONTRIBUTING.md, but maybe I wasn't looking in the right place.

It might be cool if there was some simple app that could be used for local testing with https://tilt.dev like https://github.com/hashicorp-demoapp/hashicups-setups.

I also spotted a little typo https://github.com/dapr/dapr/blob/master/pkg/diagnostics/grpc_monitoring.go#L57 healthProbeRoundripLatency should be Roundtrip along the way that bothered me enough to make a note. Probably worthy of a separate PR just to ensure consistency and searchability.

@jfreeland
Copy link
Author

I seem to recall a couple months ago that I regenerated the generated files myself but I didn't do that tonight. I don't recall seeing that I needed to in CONTRIBUTING.md, but maybe I wasn't looking in the right place.

I found @ItalyPaleAle 's reference to make code-generate and ran it again but there was no diff. I updated configuration.yaml by hand in this PR. That was probably not correct.

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