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

Makes all span/metric/event/resource groups id meaningful #1512

Merged
merged 12 commits into from
Nov 11, 2024

Conversation

lmolkova
Copy link
Contributor

@lmolkova lmolkova commented Oct 23, 2024

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, but

The group id is the common identity we can have, but it does not have any meaning now. This PR enforces the following pattern for id

  • metric.{metric_name} for metrics
  • event.{name} for events
  • resource.{name} for resources
  • span.\.+.{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.

@lmolkova lmolkova requested review from a team and tigrannajaryan as code owners October 23, 2024 22:25
@lmolkova lmolkova added the Skip Changelog Label to skip the changelog check label Oct 23, 2024
policies/yaml_schema.rego Outdated Show resolved Hide resolved
policies/yaml_schema.rego Show resolved Hide resolved
docs/feature-flags/feature-flags-logs.md Show resolved Hide resolved
Copy link
Member

@dyladan dyladan left a 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?

@lmolkova
Copy link
Contributor Author

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.

@dyladan
Copy link
Member

dyladan commented Oct 28, 2024

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 event prefix.

@MSNev
Copy link
Contributor

MSNev commented Oct 28, 2024

General Observation, while I (believe) that I understand the desire to make this change it seems a little redundant to basically have the group id prefixed with the type.

Would it not be better to just enforce this via weaver so that the id (when loaded) is always prefixed with its explicit type (so that all ids become unique based on the type. So all ids and then unique across all types and it would avoid someone trying to bypass the logic rules of "naming" a span some other type....

And then we should enforce that any references / extend to the existing id must use the fully constructed type.id, so we still enforce the isolation of the types?

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
    # ...

@lmolkova
Copy link
Contributor Author

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 event prefix.

Agreed, I'll rename to event.yaml.

@lmolkova
Copy link
Contributor Author

@MSNev I'd prefer to keep things explicit and self-contained in semconv. It'd be surprising to write http.client when defining something and then reference it as a span.http.client.

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.

@lmolkova is in danger of being an Rego expert....

I think this is a good directon forward.

@lmolkova lmolkova merged commit 52730dc into open-telemetry:main Nov 11, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog Label to skip the changelog check
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Yaml schema: unify semconv identity property
6 participants