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 resource attributes and scope to metrics proto diagram #519

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

Conversation

krisztianfekete
Copy link

Fixes #518.

// |unit | +------------------------------------+
// |data |---> |Gauge, Sum, Histogram, Summary, ... |
// +------------+ +------------------------------------+
// MetricsData
Copy link
Contributor

Choose a reason for hiding this comment

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

Why provide a definition of the MetricsData type for the Metric? This seems more appropriate in the MetricsData type documentation.

Copy link
Author

Choose a reason for hiding this comment

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

@jsuereth, @tigrannajaryan what do you think? Should I move this up?

Copy link
Member

Choose a reason for hiding this comment

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

Works either for me, as long as we have this diagram somewhere.

Copy link
Author

Choose a reason for hiding this comment

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

@tigrannajaryan, I just pushed a commit. Can you please take a look?

@krisztianfekete krisztianfekete force-pushed the add-resource-attributes-scope branch 2 times, most recently from 893bb76 to aabc9cb Compare April 10, 2024 11:32
opentelemetry/proto/metrics/v1/metrics.proto Outdated Show resolved Hide resolved
Comment on lines -100 to -107
// Metric
// +------------+
// |name |
// |description |
// |unit | +------------------------------------+
// |data |---> |Gauge, Sum, Histogram, Summary, ... |
// +------------+ +------------------------------------+
//
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't need to be removed. It contextualizes the following diagram about metric data.

Copy link
Author

Choose a reason for hiding this comment

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

I removed that based on @bogdandrutu's advice: #519 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

His comment was made when this PR was changing the documentation of this declaration. It doesn't seem relevant to me given this PR has moved the added documentation elsewhere.

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.

Improve metrics.proto by mentioning Resource Attributes and Scope
5 participants