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

+ opentelemetry: Add metrics support #1289

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hughsimpson
Copy link
Contributor

@hughsimpson hughsimpson commented Aug 4, 2023

Adds support for opentelemetry metrics reporting

Main point of discussion from my perspective is whether and how to group by 'component' before sending so that that's accurately captured.. generally I think it's a safe assumption that metrics with the same name will be from the same component, so I'm loath to spend the cycles on verifying that, but OTOH maybe I'm wrong? It could perhaps be a feature toggle down the road if necessary... Removed that bit and just tagging everything with component=kamon-metrics

There's also maybe a little more refactoring that could have been done to share a bit more of the code logic between traces and metrics, but is it really worth it? I reckon we could save about 50 lines or something at most, and it's not like there a whole whack of other telemetry facets out there waiting to be implemented. I think this is ok...

Further Edit: I haven't implemented any sort of automatic adjustment of the offsets for exponential histograms - the value for positive buckets is currently always 1, and negative always -bucketCount. I believe it should be relatively easy to implement automatic shifting of that ( half max scale minus min scale, adjust scales accordingly?) but haven't done so. I also have questions in my mind about the edge case where the original kamon metric bucket 'covers' two or more target buckets. Started to implement some distribution-of-average logic there but have held myself back for now - it should be easy enough to add, though.

There may be some inconstancy in terms of which bucket we allocate frequencies to at boundaries. I'm personally happy however with whatever error term that might introduce, although I admit to not having given it a thorough analysis

I don't know whether the spec would advocate for or against it, but it also seems reasonable to me - whether or not support for balancing base offset is added - to drop any leading 0's in the 'negative' bucket (changing its offset accordingly) and any trailing 0's in the 'positive' bucket.

@hughsimpson hughsimpson marked this pull request as draft August 4, 2023 14:50
@hughsimpson hughsimpson marked this pull request as ready for review August 4, 2023 15:56
@ivantopo
Copy link
Contributor

ivantopo commented Aug 9, 2023

Regarding this part:

Main point of discussion from my perspective is whether and how to group by 'component' before sending so that that's accurately captured.. generally I think it's a safe assumption that metrics with the same name will be from the same component, so I'm loath to spend the cycles on verifying that, but OTOH maybe I'm wrong? It could perhaps be a feature toggle down the road if necessary...

I'm my limited experience with OTel, I'm not really sure this brings any additional value to end users or makes anything easier/better along the pipeline so I would say lets group it all together as "kamon metrics" and then bring grouping per component if we have a clear use case for it. If it comes to that, we might want Kamon itself to provide a bit more information than just the component. That's another relic of the opentracing times we should kill :)

@hughsimpson
Copy link
Contributor Author

hughsimpson commented Aug 9, 2023

Fair enough on the component aspect -- it's certainly less interesting on metrics than on traces. Have pushed up a change for that

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

Successfully merging this pull request may close these issues.

2 participants