Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat: Add auto-generated semantic conventions gem #858

Merged
merged 2 commits into from
Jul 5, 2021

Conversation

ahayworth
Copy link
Contributor

Generated from the YAML, complete with Rake task to build it.

Fixes #733

Generated from the YAML, complete with Rake task to build it.
@ahayworth
Copy link
Contributor Author

Note that:

  • I didn't include tests (wasn't sure what to test)
  • I disabled a few additional rubocop rules for this, rather than fighting Jinja to produce perfect ruby style (it's close enough for an autogenerated thing, I think).

Example:

~/projects/opentelemetry-ruby/semantic_conventions ahayworth-yaml-intensifies 19s
 pry
[1] pry(main)> require_relative './lib/opentelemetry-semantic_conventions'
=> true
[2] pry(main)> OpenTelemetry::SemanticConventions::Trace::FAAS_DOCUMENT_NAME
=> "faas.document.name"
[3] pry(main)> OpenTelemetry::SemanticConventions::Resource::PROCESS_PID
=> "process.pid"
[4] pry(main)>

We could alternately put these under the OpenTelemetry::Trace and OpenTelemetry::Resource namespaces as well, if we think that's a good idea - cc @open-telemetry/ruby-approvers for any thoughts there.

Copy link
Contributor

@fbogsany fbogsany left a comment

Choose a reason for hiding this comment

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

This is awesome! Thanks @ahayworth!

@fbogsany
Copy link
Contributor

fbogsany commented Jul 5, 2021

We could alternately put these under the OpenTelemetry::Trace and OpenTelemetry::Resource namespaces as well, if we think that's a good idea

I think it is fine under OpenTelemetry::SemanticConventions. I 🤔 about whether we want additional nesting to produce names like OpenTelemetry::SemanticConventions::Trace::DB::Cassandra::KEYSPACE but I think that's getting too verbose and it'd arguably slow down lookup.

Copy link
Contributor

@robertlaurin robertlaurin left a comment

Choose a reason for hiding this comment

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

🥇

@fbogsany fbogsany merged commit d5810e3 into open-telemetry:main Jul 5, 2021
@robertlaurin
Copy link
Contributor

I think it is fine under OpenTelemetry::SemanticConventions

Agreed

I 🤔 about whether we want additional nesting to produce names like OpenTelemetry::SemanticConventions::Trace::DB::Cassandra::KEYSPACE but I think that's getting too verbose and it'd arguably slow down lookup.

I appreciate the desire for it, but that's a really long namespace.

@fbogsany
Copy link
Contributor

fbogsany commented Jul 5, 2021

I think the follow-on to this should be to remove the Resources constants from https://github.com/open-telemetry/opentelemetry-ruby/blob/main/sdk/lib/opentelemetry/sdk/resources/constants.rb and replace their use with the constants from this gem instead. This will mean the SDK depends on the semantic_conventions gem, and I think that is fine (and likely intended by the spec). We should probably be careful with the dependency so that the semantic_conventions gem can be bumped without requiring a new version of the SDK gem.

@ahayworth
Copy link
Contributor Author

Agreed about the namespacing.

@fbogsany I opened #860 for SDK follow-on work.

@ahayworth ahayworth deleted the ahayworth-yaml-intensifies branch July 7, 2021 14:37
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Semantic convention constants should be autogenerated from YAML
3 participants