Skip to content
This repository has been archived by the owner on Mar 9, 2022. It is now read-only.

Update opentelemetry deps to 0.6.0 #20

Closed
wants to merge 1 commit into from
Closed

Update opentelemetry deps to 0.6.0 #20

wants to merge 1 commit into from

Conversation

ricardoccpaiva
Copy link

  • Update opentelemetry deps to 0.6.0
  • Fix 2 compile warnings

@bryannaegele
Copy link
Collaborator

Hey, Ricardo. Did you validate against the Otel spec that no other changes are required for 0.6.0 updates? The way I usually do it is by doing diffs in the Otel spec repo between the version you're updating from to the one you're updating to, so tags 0.5.0 -> 0.6.0

@ricardoccpaiva
Copy link
Author

ricardoccpaiva commented Mar 1, 2021

Hey, Ricardo. Did you validate against the Otel spec that no other changes are required for 0.6.0 updates? The way I usually do it is by doing diffs in the Otel spec repo between the version you're updating from to the one you're updating to, so tags 0.5.0 -> 0.6.0

From my short experience with Otel and looking at this changelog, it seems that nothing is going to be not compliant with the spec but.... for some reason the exporting is failing after upgrading opentelemetry and opentelemetry_api to v0.6.0 👇

This happens when some exception is raised, i tried with a invalid query param, for instance, a string when the controller is expecting a number.

[info] exporter threw exception: exporter=:opentelemetry_exporter :error::function_clause stacktrace=[
  {:opentelemetry_exporter, :to_otlp_status, [:Error],
   [
     file: '/my_telemetry_api/deps/opentelemetry_exporter/src/opentelemetry_exporter.erl',
     line: 254
   ]},

I noticed this new field, I will try to open a PR with this new field.

@ricardoccpaiva
Copy link
Author

@bryannaegele did you see my last comment? Any thoughts or ideas?
I Will try to debug this... not sure exactly how 🤔

@bryannaegele
Copy link
Collaborator

That field isn't a required field. Looking at your error, something is failing in the exporter. Looking at that, this has to do with the changes in the spec to the status code being set.

Looking at it briefly, I think https:/opentelemetry-beam/opentelemetry_phoenix/blob/main/lib/opentelemetry_phoenix.ex#L166 needs to have the atom be lowercased to :error.

As a heads up, there is a larger issue which was discovered by Gustavo around span contexts that happened in v0.5 which I need to look into this week, so a new version may not get to hex right away.

@ricardoccpaiva
Copy link
Author

ricardoccpaiva commented Mar 8, 2021

That field isn't a required field.

I know, but we can add it anyway.

Looking at it briefly, I think https:/opentelemetry-beam/opentelemetry_phoenix/blob/main/lib/opentelemetry_phoenix.ex#L166 needs to have the atom be lowercased to :error.

I'll try that to see if it fixes the error.

Thanks for your reply, ping me back when you have some news.

@bryannaegele
Copy link
Collaborator

Closing in favor of #23. We're going to go straight to 1.0.0-rc

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants