opentelemetry-ruby icon indicating copy to clipboard operation
opentelemetry-ruby copied to clipboard

refactor: uses constants for otel.* attributes

Open Coolomina opened this issue 3 years ago • 5 comments

Takes care of #1250.

I did not find a better place for the Zipkin and Jaeger otel.* specific variables in the code. Do you think they should live under a separate file?, I guessed they belonged to the common otel.* namespace

Coolomina avatar Nov 14 '22 23:11 Coolomina

CLA Signed

The committers listed above are authorized under a signed CLA.

  • :white_check_mark: login: Coolomina / name: Alejandro Colomina (e0958af7743a920180d3c74b9b7c66796c918445)

CI / opentelemetry-exporter-otlp-common / windows-latest (pull_request) is failing due to a timeout when downlading a package. The endpoint seems to work properly now with curl https://github.com/ruby/setup-msys2-gcc/releases/download/msys2-gcc-pkgs/msys2.7z -L --output file.

Downloading msys2 build tools
  https://github.com/ruby/setup-msys2-gcc/releases/download/msys2-gcc-pkgs/msys2.7z
  connect ETIMEDOUT 185.199.110.133:443
  Waiting 10 seconds before trying again
  connect ETIMEDOUT 185.199.110.133:443
  Waiting 12 seconds before trying again

Any of the reviewers to give it a retry maybe ? //cc @arielvalentin 😄

Coolomina avatar Nov 15 '22 09:11 Coolomina

For OpenTelemetry::Exporters::* I created the consensus of having an OpenTelemetry::Exporter::#{name}::Attributes because it felt wrong to put the constants in the main required file (ej: exporter/jaeger/lib/opentelemetry/exporter/jaeger.rb) and reference them without context in another file.

Do you think referencing them with Attributes::ATTR_NAME makes sense?

For the BatchSpanProcessor I just put them in the class as non-private.

Let me know if further edits are needed!

Coolomina avatar Nov 18 '22 21:11 Coolomina

@Coolomina This looks good to me, but I think there might be two more small changes:

  1. I believe you need to update the gemspecs for everything where you added the new dependency on the opentelemetry-semantic_conventions gem. The tests pass because we install lots of things locally in CI, but that could break in the actual released versions. (I think it needs to be added, anyways; I could be wrong).
  2. In the ./semantic_conventions directory, can you run bundle exec rake generate and make sure your changes aren't overwritten? Most of that gem is actually generated from upstream specifications, so I just want to ensure things don't break. I think it will be okay though.

ahayworth avatar Nov 21 '22 22:11 ahayworth

@ahayworth Thanks for the patience :D.

Having added the dependency in exporter/otlp-http/opentelemetry-exporter-otlp-http.gemspec, the results do not seem to overwrite anything.

opentelemetry-ruby/semantic_conventions [otel-constants●] » bundle exec rake generate
...
...
Using default SPAN type for semantic convention 'webengine_resource' @ line 5

opentelemetry-ruby/semantic_conventions [otel-constants] » echo $?; git status      
0
On branch otel-constants
nothing to commit, working tree clean

Coolomina avatar Nov 22 '22 19:11 Coolomina

👋 This pull request has been marked as stale because it has been open with no activity. You can: comment on the issue or remove the stale label to hold stale off for a while, add the keep label to hold stale off permanently, or do nothing. If you do nothing this pull request will be closed eventually by the stale bot

github-actions[bot] avatar Mar 10 '24 01:03 github-actions[bot]