-
Notifications
You must be signed in to change notification settings - Fork 169
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
Makes all span/metric/event/resource groups id meaningful #1512
base: main
Are you sure you want to change the base?
Conversation
@@ -1,11 +1,14 @@ | |||
groups: | |||
- id: log-feature_flag |
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.
that's effectively the same change as made in the https://github.com/open-telemetry/semantic-conventions/pull/1440/files#diff-1db9b9bcfb009755e349125b55d3c945e2b5636723e91ee73ac4cd9184108036
@@ -1,14 +0,0 @@ | |||
groups: | |||
- id: feature_flag |
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.
one of identical feature_flag events has to go so that we can have one group id that follows the pattern.
Removing it also partially unblocks open-telemetry/weaver#306
|
||
| Attribute | Type | Description | Examples | [Requirement Level](https://opentelemetry.io/docs/specs/semconv/general/attribute-requirement-level/) | Stability | | ||
|---|---|---|---|---|---| | ||
| [`feature_flag.key`](/docs/attributes-registry/feature-flag.md) | string | The unique identifier of the feature flag. | `logo-color` | `Recommended` | ![Experimental](https://img.shields.io/badge/-experimental-blue) | | ||
| [`feature_flag.key`](/docs/attributes-registry/feature-flag.md) | string | The unique identifier of the feature flag. | `logo-color` | `Required` | ![Experimental](https://img.shields.io/badge/-experimental-blue) | |
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.
Is the change from recommended to required here intentional? 🤔
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.
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 wasn't sure when to have an events.yaml and when to have a logs.yaml. Is there anywhere documentation which describes when to use which one?
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 think it should be event.yaml, but really the file name is not enforced or regulated.
The convention itself should be for events because of
- A semantic convention needs to be defined. We do not define semantic
conventions for LogRecords that are not Events.
Let me create an issue so we document our stance on span events vs events vs logs in semantic conventions
3841616
to
9313ca9
Compare
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.
Approving from feature_flag perspective. What is the reason you removed event.yaml
and kept log.yaml
instead of the other way around as it appears in most other semconvs? Is that just to mimic what I did in my other PR?
Thanks @dyladan ! Yes, just to mimic what you've done in the other PR. |
OK. For consistency with other areas of semconv should we use events.yaml instead? Looks like most are using that and the id you used is using the |
General Observation, while I (believe) that I understand the desire to make this change it seems a little redundant to basically have the group Would it not be better to just enforce this via weaver so that the And then we should enforce that any references / extend to the existing So as an example of the change the following would be what was required - id: the.group.id # resulting id becomes span.the.group.id.client
type: span
brief: group brief
extends: span.the.other.group.id.client # You still MUST use the fully qualified group id here
span_kind: client
attributes:
- ref: aws.dynamodb.table_names
- id: the.other.group.id
type: span
span_kind: client
# ... |
Fixes #1324
Part of #1472
In order to provide stability guarantees, we need to make semconv groups identifiable.
We currently have different identifying properties -
name
on event,metric_name
on metric,name
on resource, butsemconv_grouped_metrics
group by root namespace uses group.id instead of metric_name to determine root namespace weaver#289dns.lookup.duration
could be a metric and an attribute)The group
id
is the common identity we can have, but it does not have any meaning now. This PR enforces the following pattern forid
metric.{metric_name}
for metricsevent.{name}
for eventsresource.{name}
for resourcesspan.\.+.{span_kind}
for spans - spans don't have identifying property, so there is nothing to check at the moment. We should be able to change this format if we introduce something better.