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

Clarify Attributes advisory parameter #4048

Conversation

pichlermarc
Copy link
Member

@pichlermarc pichlermarc marked this pull request as ready for review May 14, 2024 11:41
@pichlermarc pichlermarc requested review from a team as code owners May 14, 2024 11:41
@@ -276,7 +276,7 @@ boundaries to use if aggregating to
Applies to all instrument types.

`Attributes` (a list of [attribute keys](../common/README.md#attribute)) is
the recommended set of attribute keys to be used for the resulting metrics.
the set of allowed attribute keys that may be used on the resulting metrics.
Copy link
Contributor

Choose a reason for hiding this comment

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

First - MAY is normative and should be capitalized when used.

Second - I'm not sure this improves the understanding of what will happen here.

It is is true that this set of attribute keys MAY be used by the SDK, and only attributes found in this set will "pass" through if this set is used. However "allowed" implied that we'd erase measurements that contain other attribute keys, which isn't true.

In the SDK portion of this, we explicitly state that other attribtutes are ignored when using the term "allow list".

I'd suggest we use very similar terminology to the SDK here:

 This is an allow-list of attribute keys for measurements captured in the metric stream that MAY be used. When used, the allow-list contains attribute keys that identify the attributes that MUST be kept, and all other attributes MUST be ignored.

Possibly we could link to the SDK here, since I'm not sure we've introduced the term "metric stream" yet.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep that makes sense. I applied the suggestion in 9672d5b and linked the same data model subsection as the SDK does when it introduces the concept of a metrics stream.

@reyang reyang added the spec:metrics Related to the specification/metrics directory label May 16, 2024
Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label May 24, 2024
the recommended set of attribute keys to be used for the resulting metrics.
`Attributes` (a list of [attribute keys](../common/README.md#attribute)) is an
allow-list of attribute keys for measurements captured in the [metric stream](../metrics/data-model.md#events--data-stream--timeseries)
that MAY be used. When used, the allow-list contains attribute keys that
Copy link
Member

Choose a reason for hiding this comment

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

that MAY be used. When used,

It is not clear what does "use" means here?

The SDK side of the spec also feels a bit odd to me..
Is the Attributes advisory parameter used by default in SDK?
or user must explicitly opt-in to say "take attributes from attributes advisory, if any. else take everything"?
(This maybe outside of the this PR's scope, but I think clarifying the SDK behavior is important before tweaking the wording here...)

Comment on lines +279 to +283
`Attributes` (a list of [attribute keys](../common/README.md#attribute)) is an
allow-list of attribute keys for measurements captured in the [metric stream](../metrics/data-model.md#events--data-stream--timeseries)
that MAY be used. When used, the allow-list contains attribute keys that
identify the attributes that MUST be kept, and all other attributes MUST be
ignored.
Copy link
Member

Choose a reason for hiding this comment

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

(this is tricky, also not sure MUST is correct since it's just "advice")

Suggested change
`Attributes` (a list of [attribute keys](../common/README.md#attribute)) is an
allow-list of attribute keys for measurements captured in the [metric stream](../metrics/data-model.md#events--data-stream--timeseries)
that MAY be used. When used, the allow-list contains attribute keys that
identify the attributes that MUST be kept, and all other attributes MUST be
ignored.
`Attributes` (a list of [attribute keys](../common/README.md#attribute)) is
the recommended set of attribute keys to be retained in the resulting metrics.

Copy link

github-actions bot commented Jun 5, 2024

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Jun 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec:metrics Related to the specification/metrics directory Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clarify Attributes instrument advisory parameter
5 participants