open-telemetry / opentelemetry-erlang

OpenTelemetry Erlang SDK
https://opentelemetry.io
Apache License 2.0
330 stars 105 forks source link

Bump opentelemetry_api from 1.2.1 to 1.2.2 breaks #608

Open epinault opened 1 year ago

epinault commented 1 year ago

I am trying to apply a patch but my build get this error

what changed that could cause this? 1.2.1 worked fine. So something underlying is breaking now this is with Elixir 1.14 and OTP 25

7:03:44.100 [notice] Application opentelemetry exited: exited in: :opentelemetry_app.start(:normal, [])
    ** (EXIT) an exception was raised:
        ** (UndefinedFunctionError) function :otel_tracer_provider_sup.start/2 is undefined or private
            (opentelemetry 1.3.0) :otel_tracer_provider_sup.start(:global, %{attribute_count_limit: 128, attribute_per_event_limit: 128, attribute_per_link_limit: 128, attribute_value_length_limit: :infinity, bsp_exporting_timeout_ms: :undefined, bsp_max_queue_size: :undefined, bsp_scheduled_delay_ms: :undefined, create_application_tracers: true, deny_list: [], event_count_limit: 128, id_generator: :otel_id_generator, link_count_limit: 128, log_level: :info, metrics_exporter: {:opentelemetry_exporter, %{}}, processors: [otel_batch_processor: %{exporter: {:opentelemetry_exporter, %{}}, exporting_timeout_ms: 30000, max_queue_size: 2048, scheduled_delay_ms: 5000}], readers: [], register_loaded_applications: :undefined, resource_detector_timeout: 5000, resource_detectors: [:otel_resource_env_var, :otel_resource_app_env], sampler: {:parent_based, %{root: :always_on}}, ssp_exporting_timeout_ms: :undefined, sweeper: %{interval: 600000, span_ttl: 1800000, storage_size: :infinity, strategy: :drop}, text_map_propagators: [:trace_context, :baggage], traces_exporter: {:opentelemetry_exporter, %{}}, views: []})
            (opentelemetry 1.3.0) /code/deps/opentelemetry/src/opentelemetry_app.erl:39: :opentelemetry_app.start/2
            (kernel 8.5.4) application_master.erl:293: :application_master.start_it_old/4
tsloughter commented 1 year ago

What SDK version are you using?

epinault commented 1 year ago

what do you mean by SDK? what the package you are looking for?

tsloughter commented 1 year ago

opentelemetry

smoggach-nl commented 1 year ago

Same here. opentelemetry version is 1.2.1

tsloughter commented 1 year ago

Pls try opentelemetry 1.3.1

smoggach-nl commented 1 year ago

it works with that version of opentelemetry 👍

epinault commented 1 year ago

same here, works with 1.3.1. Problem is tooling like Dependabot had 2 package changes so seems that it caused the problem .Once I upgraded the 1.3.1 and rebased the other one , it was all good

andrewhr commented 1 year ago

For context. Updating both does fix the issue, but it's tricky with automation tools like dependabot, unfortunately.

@tsloughter if possible, maybe a good move is to retire the API 1.2.2 and introduce a new version which forces minimal version on the SDK. That should help, I assume.

tsloughter commented 1 year ago

The API doesn't and can't depend on the SDK.

tsloughter commented 1 year ago

Would it have worked out if I had properly bumped the version numbers as minors instead of patches?

tsloughter commented 1 year ago

Not that it would have forced the SDK upgrade to be included but it maybe would have been handled different by tooling?

andrewhr commented 1 year ago

The API doesn't and can't depend on the SDK.

Right, I completely forgot about that!

Would it have worked out if I had properly bumped the version numbers as minors instead of patches?

I think everyone just assumes the library is 100% semver, which in here wasn't the case. So minor bump would not help :/

tsloughter commented 1 year ago

Yea. "semver". Definitely want to follow it as much as possible, and believe the Otel project requires it, but mistakes are made.

We keep backwards compatible functionality throughout the API to follow semver, the issue with my thinking here was that it is a call I thought of as only the API made to the SDK and not by users, so it wouldn't be noticed. But that doesn't work in this situation where the API doesn't actually depend on the SDK so it can't define a version constraint :)

The function that actually calls this code, start_tracer_provider was deprecated instead of removed for this very semver reason but apparently I failed at actually keeping it backwards compatible, haha.

Sorry about the issue, clearly need some testing setup that will verify the new API works with older SDKs.

epinault commented 1 year ago

no worries . it s all fixed up for now for us. And yea dependabot can be tricky sometime for some packag

and hello @andrewhr :)