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 metrics spec supplementary guidelines #1966

Merged
merged 19 commits into from
Sep 30, 2021

Conversation

reyang
Copy link
Member

@reyang reyang commented Sep 24, 2021

Related to #1873 and #1891.

If you find it hard to look at the raw markdown file, here is a rendered version https://github.com/reyang/opentelemetry-specification/blob/reyang/guideline/specification/metrics/supplementary-guidelines.md.

@reyang reyang requested review from a team as code owners September 24, 2021 20:40
@reyang reyang added area:sdk Related to the SDK spec:metrics Related to the specification/metrics directory labels Sep 24, 2021
@reyang reyang added this to the Metrics API/SDK Feature Freeze milestone Sep 24, 2021
Copy link
Contributor

@jsuereth jsuereth left a comment

Choose a reason for hiding this comment

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

This generally looks very good. I need to take a second pass on grammar/phrasing (That's my own worst enemy), but the content looks like a great addition/treatment of the issue for implementers.

One thing I'd like to call out though is a suggestion around Memory Management and Allocations. While it's impossible to remove all allocations on the hot path of synchronous metric collection, particularly if using Baggage, SDK authors should be encouraged to reduce memory contention in the hot path, and pay most overhead at startup. This aligns with OTel's vision of "least disruption to runtime", which shows up in recommendations around error-reporting during operation and likely should also apply to things like memory usage.

I like the idea of limiting memory as suggested here, just want to also reinforce this "allocate ahead of time and reuse" notion, which we actually expose a bit more directly in Java (and I need to annotate).

Specifically, one scenario to call out (more important for Logs/Traces than metrics) is how a system behaves when memory can no longer be allocated. Do we still get observability or is it gone?

@reyang
Copy link
Member Author

reyang commented Sep 27, 2021

I like the idea of limiting memory as suggested here, just want to also reinforce this "allocate ahead of time and reuse" notion, which we actually expose a bit more directly in Java (and I need to annotate).

I've updated the PR to cover the pre-allocation for garbage collected languages. PTAL.

Specifically, one scenario to call out (more important for Logs/Traces than metrics) is how a system behaves when memory can no longer be allocated. Do we still get observability or is it gone?

I've updated the PR trying to scratch the surface. If we need to dig deeper, a separate PR might be better?

Co-authored-by: Alan West <3676547+alanwest@users.noreply.github.com>
Copy link
Contributor

@jmacd jmacd left a comment

Choose a reason for hiding this comment

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

❤️ ❤️ ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:sdk Related to the SDK spec:metrics Related to the specification/metrics directory
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

6 participants