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 experimental go runtime metrics semantic conventions #981

Merged
merged 20 commits into from
May 23, 2024

Conversation

dashpole
Copy link
Contributor

@dashpole dashpole commented Apr 29, 2024

Fixes #535

Changes

Add experimental runtime metrics based on the proposed set of recommended metrics from the Go team: golang/go#67120

Please direct feedback on the set of metrics on the issue above. If the recommended set is changed, I will update these conventions. Feedback on the OTel representation of those metrics is welcome.

Alternatives considered

Separate metrics for memory usage

Instead of a single go.memory.usage metric, we could have one metric for the total, and other metrics for the stack and released amounts. Separate metrics would potentially be more "future proof", if a new sub-set of memory that overlapped with released or stack memory was ever introduced. I chose the existing design because using labels is easier to visualize, and understand for users.

go.gc.* instead of go.memory.gc.*

This PR uses go.memory.gc.*. Instead, we could use go.gc.*, similar to java's jvm.gc.duration. I went this direction because it is garbage collecting memory, and I felt it was most appropriate to compare the GC target to the go.memory.usage and go.memory.limit metrics.

Different naming for "total" memory metrics

/gc/heap/allocs:bytes and /gc/heap/allocs:objects from the go runtime are both cumulative totals for number and size of allocations made. I've opted to name them go.memory.allocated and go.memory.allocations to be concise, but users may think go.memory.allocated sounds like the same thing as go.memory.usage. Some alternative names I considered:

  • go.memory.size_allocated and go.memory.objects_allocated

Note that we cannot use the count or total suffixes per https://github.com/open-telemetry/semantic-conventions/blob/main/docs/general/metrics.md#naming-rules-for-counters-and-updowncounters

go.config.gomemlimit instead of go.memory.limit

To be consistent with the other configuration metric, go.config.gogc, we could name the memory limit go.config.gomemlimit. I've opted for the go.memory.limit naming because it aligns with jvm.memory.limit, and because it helps users understand that they should compare memory usage with the limit.

@open-telemetry/go-approvers @felixge @mknyszek

@dashpole dashpole force-pushed the runtime_metrics branch 3 times, most recently from 59b9652 to 4b258a1 Compare April 29, 2024 19:11
@dashpole dashpole marked this pull request as ready for review April 29, 2024 19:39
@dashpole dashpole requested review from a team as code owners April 29, 2024 19:39
Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

LGTM with some non-blocking clarification questions.

docs/runtime/go-metrics.md Outdated Show resolved Hide resolved
docs/runtime/go-metrics.md Show resolved Hide resolved
docs/runtime/go-metrics.md Outdated Show resolved Hide resolved
docs/runtime/go-metrics.md Outdated Show resolved Hide resolved
@trask
Copy link
Member

trask commented Apr 29, 2024

Add experimental runtime metrics based on the proposed set of recommended metrics from the Go team: https://docs.google.com/document/d/1NBeEYxld5lA_UUXoFniBO0a6_ryHYgFC0hgUAjpfdDY/edit?resourcekey=0-NBd-0IZAYTtiPej124rR2w&tab=t.0

Can this doc be made publicly accessible?

Copy link
Member

@felixge felixge left a comment

Choose a reason for hiding this comment

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

Thanks for raising this @dashpole . But as mentioned by others, the document you linked hasn't been made public yet. It might also make sense to allow some time for discussing the contents of the document in the Go community. (But since your PR here uses "status: experimental" I guess I don't feel strongly about it)

docs/runtime/go-metrics.md Show resolved Hide resolved
docs/runtime/go-metrics.md Outdated Show resolved Hide resolved
docs/runtime/go-metrics.md Outdated Show resolved Hide resolved
docs/runtime/go-metrics.md Outdated Show resolved Hide resolved
@dashpole
Copy link
Contributor Author

Can this doc be made publicly accessible?

I don't believe it can be. Feel free to request access.

@dashpole
Copy link
Contributor Author

But as mentioned by others, the document you linked hasn't been made public yet. It might also make sense to allow some time for discussing the contents of the document in the Go community. (But since your PR here uses "status: experimental" I guess I don't feel strongly about it)

I'm hoping this will help provide feedback on the proposal. These conventions will be kept experimental until the proposal is finalized.

@mknyszek
Copy link

I have posted the proposal on the Go issue tracker; no need to look at the doc anymore. Please comment at golang/go#67120.

@dashpole dashpole force-pushed the runtime_metrics branch 2 times, most recently from a0a8155 to c24db68 Compare May 1, 2024 20:11
@dashpole dashpole force-pushed the runtime_metrics branch 3 times, most recently from 1d49f7d to d470474 Compare May 6, 2024 15:42
model/registry/go.yaml Outdated Show resolved Hide resolved
Copy link

linux-foundation-easycla bot commented May 17, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@dashpole
Copy link
Contributor Author

@reyang anything else needed for this to merge?

@reyang reyang merged commit c54b6c8 into open-telemetry:main May 23, 2024
12 checks passed
@reyang
Copy link
Member

reyang commented May 23, 2024

@reyang anything else needed for this to merge?

Nope, you're all set 👍

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

Successfully merging this pull request may close these issues.

Semantic conventions for go runtime metrics
8 participants