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 missing attributes to otel semantic conventions mapping docs #20967

Closed
wants to merge 3 commits into from

Conversation

ringerc
Copy link

@ringerc ringerc commented Dec 11, 2023

What does this PR do? What is the motivation?

Fix incomplete documentation on otel semantic conventions mappings.

The otel semantic conventions mapping docs are missing a variety of attributes.

I've added some of them here. More are missing; see https://github.com/DataDog/opentelemetry-mapping-go/blob/5432be02915e24c7e8a294cd4a17877016f9b73d/pkg/otlp/attributes/attributes.go#L56

DD should really make sure this doc matches the implementation.

Merge instructions

  • Please merge after reviewing

Additional notes

It'd be best if you added the rest of the missing attributes on top of my PR.

The otel semantic conventions mapping docs are missing a variety of attributes.

I've added some of them here. More are missing; see https://github.com/DataDog/opentelemetry-mapping-go/blob/5432be02915e24c7e8a294cd4a17877016f9b73d/pkg/otlp/attributes/attributes.go#L56 

DD should really make sure this doc matches the implementation.
@ringerc ringerc requested a review from a team as a code owner December 11, 2023 03:40
@alai97
Copy link
Contributor

alai97 commented Dec 11, 2023

Thanks for creating this PR, @ringerc! I created an editorial review card and a writer will review + add onto your PR.

@alai97 alai97 added the editorial review Waiting on a more in-depth review label Dec 11, 2023
@ringerc
Copy link
Author

ringerc commented Dec 11, 2023

Please see also DataDog/opentelemetry-mapping-go#156

@@ -60,10 +65,22 @@ For more information, see [Unified Service Tagging][2].
| `k8s.cronjob.name` | `kube_cronjob` |
| `k8s.namespace.name` | `kube_namespace` |
| `k8s.pod.name` | `pod_name` |
| `app.kubernetes.io/name` | `kube_app_name` |
Copy link
Member

Choose a reason for hiding this comment

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

Although I agree we need to document this somewhere, I don't think this is the right place. This table is for the mapping from OTel Conventions to DD conventions, but the additions here are not OTel conventions.

Perhaps we can create a new table at the bottom where the left column name is "Kubernetes Labels" or "Kubernetes Conventions" instead of "OpenTelemetry convention".

What do you think ?

Copy link
Author

Choose a reason for hiding this comment

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

Sounds good. Feel free to take this as the basis for a docs fix.

@mackjmr
Copy link
Member

mackjmr commented Dec 12, 2023

Thank you for putting up the PR! Just a small nit.

Copy link
Collaborator

@kayayarai kayayarai left a comment

Choose a reason for hiding this comment

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

Thank you for this contribution! Left you some feedback about link formatting conventions for the docs site.

Comment on lines +77 to +79
* [Datadog: OpenTelemetry metric types][3]
* [Datadog: OpenTelemetry Metric mappings][4]
* [Implementation of these mappings in the exporter](https://github.com/DataDog/opentelemetry-mapping-go/blob/main/pkg/otlp/attributes/attributes.go)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Further reading links go in the yaml frontmatter (see lines 4-7 above)

content/en/opentelemetry/guide/semantic_mapping.md Outdated Show resolved Hide resolved
Co-authored-by: Kari Halsted <12926135+kayayarai@users.noreply.github.com>
@ringerc
Copy link
Author

ringerc commented Jan 16, 2024

I can't do more to update DD's docs right now. Feel free to close this PR and make another.

@kayayarai
Copy link
Collaborator

I can't do more to update DD's docs right now. Feel free to close this PR and make another.

No problem! Thanks for the start, we'll get this in.

@kayayarai
Copy link
Collaborator

Moved to #21434

@kayayarai kayayarai closed this Jan 18, 2024
@ringerc
Copy link
Author

ringerc commented Jan 23, 2024

Thanks very much

@ringerc ringerc deleted the patch-1 branch January 23, 2024 02:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editorial review Waiting on a more in-depth review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants