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.

Sign up for GitHub

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

Jump to bottom

Add generated modules that output const &str for tracing compatibility #1334

Merged
merged 5 commits into from
Nov 12, 2023
Merged

Add generated modules that output const &str for tracing compatibility #1334

merged 5 commits into from
Nov 12, 2023

Conversation

kriswuollett
Copy link
Contributor

@kriswuollett kriswuollett requested a review from a team November 1, 2023 16:52
@kriswuollett
Copy link
Contributor Author

Still need to test with my own project for both correctness and devex.

@codecov Codecov
Copy link

codecov bot commented Nov 1, 2023 •
edited
Loading

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Files Coverage Δ
opentelemetry-jaeger/src/exporter/config/mod.rs 79.7% <100.0%> (ø)
opentelemetry-stackdriver/src/lib.rs 32.2% <100.0%> (+1.3%) ⬆️
opentelemetry-datadog/src/exporter/mod.rs 50.7% <50.0%> (ø)
opentelemetry-zipkin/src/exporter/mod.rs 0.0% <0.0%> (ø)

... and 15 files with indirect coverage changes

📢 Thoughts on this report? Let us know!

shaun-cox
shaun-cox approved these changes Nov 1, 2023
View reviewed changes
Copy link
Contributor

@shaun-cox shaun-cox left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

lalitb
lalitb reviewed Nov 3, 2023
View reviewed changes
opentelemetry-semantic-conventions/src/semconv/mod.rs Outdated
@@ -0,0 +1,2 @@
pub mod resource;
Copy link
Member

@lalitb lalitb Nov 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Do we need this semconv directory along with the generated files here, now that we are regenerating the existing files?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No we don't need the dir. Removed it my recent force push. Will take off the WIP soon once I double check the tests and other code.

@kriswuollett kriswuollett force-pushed the add-str-semconv-keys branch 4 times, most recently from b1f92bf to 66019dd Compare November 6, 2023 17:39
@kriswuollett kriswuollett changed the title [WIP] Add generated modules that output const &str for tracing compatibility Add generated modules that output const &str for tracing compatibility Nov 6, 2023
@kriswuollett kriswuollett mentioned this pull request Nov 6, 2023
Closed
jtescher
jtescher requested changes Nov 7, 2023
View reviewed changes
Copy link
Member

@jtescher jtescher left a comment

Choose a reason for hiding this comment

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

Overall supportive of this change, some nits left

opentelemetry-semantic-conventions/src/semconv/resource.rs Outdated
@@ -0,0 +1,882 @@
// DO NOT EDIT, this is an auto-generated file
Copy link
Member

Choose a reason for hiding this comment

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

Was this file added by mistake? seems to be a dup of src/resource.rs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Left in by mistake, removing.

opentelemetry-semantic-conventions/src/semconv/trace.rs Outdated Show resolved Hide resolved
opentelemetry-semantic-conventions/scripts/templates/header_resource.rs Outdated Show resolved Hide resolved
cijothomas
cijothomas previously requested changes Nov 8, 2023
View reviewed changes
Copy link
Member

@cijothomas cijothomas left a comment

Choose a reason for hiding this comment

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

Changes looks good!

I am leaving my review as "request-changes", specifically for addressing the below points:
https://github.com/open-telemetry/opentelemetry-rust/pull/1334/files#r1387150519
https://github.com/open-telemetry/opentelemetry-rust/pull/1334/files#r1387146406

@cijothomas cijothomas dismissed their stale review November 10, 2023 16:50

Comments addressed! Thank you!

@TommyCpp TommyCpp merged commit dd03fda into open-telemetry:main Nov 12, 2023
@djc djc mentioned this pull request Feb 21, 2024
Closed
cijothomas pushed a commit to cijothomas/opentelemetry-rust that referenced this pull request Sep 7, 2024
@kriswuollett @hdost
Add generated modules that output const &str for tracing compatibil…
fa0a3bb 
…ity (open-telemetry#1334)

* feat: add generated modules that output const &str for tracing compatibility

* fix: add tracing as a dev-dependency for doc examples

* fix: remove unused code

* fix: remove tracing examples from semconv docs

* docs: remove extra whitespace that was added previously
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lalitb lalitb lalitb left review comments

@jtescher jtescher jtescher approved these changes

@shaun-cox shaun-cox shaun-cox approved these changes

@cijothomas cijothomas cijothomas left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Need &'static strs from semantic-conventions for tracing field recording
6 participants
@kriswuollett @jtescher @lalitb @cijothomas @shaun-cox @TommyCpp