-
Notifications
You must be signed in to change notification settings - Fork 576
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
Runtime metrics plugin #9
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm willing to get this merged nearly as-is, while we work on batch observers, the CumulativeObserver instrument, and SDK support for exporting deltas from cumulatives, all of which will be needed before this can be done the way we'd like. Let's try and get the API settled now--I propose naming it runtime.Start()
and returning an error instead of panicking, the rest of the issues will be addressed after more work on the API/SDK.
FYI this will help: open-telemetry/opentelemetry-go#634 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm inclined to get this merged because it's clearly useful and ready to use. There are two concerns I have: (1) there will be changes when new instruments from the 0.4 specification are introduced, that will suggest internal reorganization of this code, (2) there will be an effort to standardize on OTel runtime metrics names, which may result in changing the conventional metrics names for runtime metrics.
(1) is not a problem, and we will take care of updating this PR after the new instruments are ready.
(2) could be a problem, @chrisleavoy what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great. I'd like to avoid any more technical changes and get this merged, but some comments would be nice. After this merges, we can update this to use new the instruments coming in the 0.4 OTel metrics specification, and things will change a bit. (I'll be glad to take this over.)
Does this package need to be in |
Can anyone share what is required to get circleci working, if needed for this PR? |
The CircleCI config had been in the wrong place. I merged master into your branch which should fix that. |
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
|
||
// poll now so that the first tick has a full delta | ||
r.numCgoCalls = goruntime.NumCgoCall() | ||
goruntime.ReadMemStats(&r.memStats) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@quentinmit, reading the meeting nodes of the metrics sig, you mentioned that this ReadMemStats function (GC) "is no longer expensive" in Go? Its still labeled as stop-the-world however. I'm curious to hear from folks who have more expertise than me in this area.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found this: https://go-review.googlesource.com/c/go/+/34937/
(Not sure..)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's the change I was thinking of. Also, I believe ReadGCStats does not STW at all.
* master: Runtime metrics plugin (open-telemetry#9) Fix issues raised by golangci-lint (open-telemetry#29) use correct circleci dir (open-telemetry#30) gorilla/mux instrumentation (open-telemetry#19) Update CODEOWNERS and CONTRIBUTING.md to match otel-go (open-telemetry#27) Dogstatsd exporter resource support (for 0.4.3 release) (open-telemetry#25) add datadog metrics exporter (open-telemetry#22) Update codeowners and maintainers (open-telemetry#21) # Conflicts: # go.mod # go.sum # internal/trace/http.go
* Work in progress from https://github.com/lightstep/opentelemetry-golang-prototype * Renames * Rename * Finish rename * Rename packages * README
No description provided.