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.

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

fix tracer configuration merging #221

Merged
merged 3 commits into from
Mar 16, 2021

Conversation

tsloughter
Copy link
Member

Ran into this while working on some examples for docs.

This patch also makes the default exporter be undefined. I'm not sure what the best is here, when its defaulting to OTLP it crashes bad (confusing). So maybe it should instead be stdout or OTLP but handle the crash better to log a message to let the user it tried using OTLP but something wasn't configured or available.

@ferd
Copy link
Member

ferd commented Mar 11, 2021

Approved the changeset on intent. All tests failing is necessary to fix first.

That being said, the idea of setting OTLP and getting crash logs explaining what is missing in the config would likely be best for someone wanting it to work in most cases, assuming their intent is ultimately to have that exporter going. For the purposes of docs, would having a "demo" stdout exporter make sense to configure rather than defaulting to nothing?

@tsloughter
Copy link
Member Author

Ugh, its the libwxgtk actions change I saw someone mention on twitter.

@tsloughter
Copy link
Member Author

@tsloughter
Copy link
Member Author

Woops, pinged Erlanger Paul Oliver instead of Erlanger @paulo-ferraz-oliveira :)

@tsloughter
Copy link
Member Author

Aggh, erlef/setup-elixir is also broken but because of libcrypto.

@codecov
Copy link

codecov bot commented Mar 11, 2021

Codecov Report

Merging #221 (dd2620c) into main (ff12ebb) will decrease coverage by 0.12%.
The diff coverage is 16.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #221      +/-   ##
==========================================
- Coverage   34.96%   34.84%   -0.13%     
==========================================
  Files          37       37              
  Lines        3037     3045       +8     
==========================================
- Hits         1062     1061       -1     
- Misses       1975     1984       +9     
Flag Coverage Δ
api 61.11% <ø> (ø)
elixir 18.05% <ø> (ø)
erlang 34.69% <16.66%> (-0.13%) ⬇️
exporter 19.54% <ø> (ø)
sdk 77.47% <16.66%> (-1.34%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
apps/opentelemetry/src/opentelemetry_sup.erl 100.00% <ø> (ø)
apps/opentelemetry/src/otel_batch_processor.erl 67.34% <9.09%> (-7.66%) ⬇️
apps/opentelemetry/src/otel_configuration.erl 79.71% <100.00%> (+1.76%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ff12ebb...dd2620c. Read the comment docs.

@tsloughter
Copy link
Member Author

So I've updated it to have some useful warning logs if the exporter fails to initialize.

I'm still unsure if we should default to OTLP exporter or something like stdout. Defaulting to the OTLP exporter, which has default host and port does make for simpler configuration in demos/guides when exporting to a local collector, but maybe that is actually worse since most people in prod will not have the collector on localhost.

@paulo-ferraz-oliveira
Copy link

Yeah, for the libwx stuff I just "downgraded" to ubuntu-18.04. I'm not sure if it's solved in gleam-lang/setup-erlang, at the moment. But I believe I tried the latest version and other issues arose. Ah, yeah, it was the fetching of versions from Erlang Solutions that aren't there.

@puzza007
Copy link
Contributor

Woops, pinged Erlanger Paul Oliver instead of Erlanger @paulo-ferraz-oliveira :)

335134

@tsloughter tsloughter merged commit 9f0d961 into open-telemetry:main Mar 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants