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

Add missing config files (including latest configuration, as per http… #1120

Merged
merged 4 commits into from
Jul 18, 2023

Conversation

holly-cummins
Copy link
Contributor

Fixes #1114

I haven't changed any function, only added the three main config files referenced in https://github.com/quarkusio/quarkus/blob/main/docs/src/main/asciidoc/opentelemetry.adoc into the quickstart source code so the out-of-the-box experience is better.

I've manually confirmed these steps work with the new source (and no manual extra file creation):

docker-compose up -d
quarkus dev
curl http://localhost:8080/hello

and that http://localhost:16686/ then shows the traces.

Copy link
Member

@rsvoboda rsvoboda left a comment

Choose a reason for hiding this comment

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

PRs for quarkus-quickstarts repo need to target development branch

Please adjust the target and keep just 60f6ace
(there are 4 commits in the PR)

@gsmet gsmet changed the base branch from main to development September 5, 2022 18:51
@gsmet gsmet requested a review from rsvoboda September 5, 2022 18:53
@gsmet
Copy link
Member

gsmet commented Sep 5, 2022

I fixed the PR.

@rsvoboda
Copy link
Member

rsvoboda commented Sep 5, 2022

I received this error when running docker-compos up

otel-collector_1     | Error: failed to get config: cannot unmarshal the configuration: error reading exporters configuration for "jaeger": 1 error(s) decoding:
otel-collector_1     |
otel-collector_1     | * '' has invalid keys: insecure
otel-collector_1     | 2022/09/05 21:08:11 collector server run finished with error: failed to get config: cannot unmarshal the configuration: error reading exporters configuration for "jaeger": 1 error(s) decoding:
otel-collector_1     |
otel-collector_1     | * '' has invalid keys: insecure
opentelemetry-quickstart_otel-collector_1 exited with code 1

MBP 2019, macOS Monterey

@holly-cummins
Copy link
Contributor Author

@radcortez, any thoughts? This is now well past my otel expertise :)

@rsvoboda
Copy link
Member

rsvoboda commented Sep 5, 2022

open-telemetry/opentelemetry-collector#4063 changed the way this setting is specified (it is now under the tls section)

Docs need to be updated imho too
Docs were fixed in quarkusio/quarkus@62f6e4f

@radcortez
Copy link
Member

@radcortez, any thoughts? This is now well past my otel expertise :)

As @rsvoboda the jaeger exporter changed the configuration. If you get an error with the new configuration, you may need to run docker-compose pull first to update it.

@famod
Copy link
Member

famod commented Nov 30, 2022

Any updates on this? I just wanted to play around with that quickstart and was surprised by its sparseness.

@holly-cummins
Copy link
Contributor Author

Thanks @famod, @rsvoboda, @radcortez. I lost track of this, but it's still needed as far as I can tell, so we should try and get it in. I've applied @famod's and @rsvoboda's suggestions, and rebased the PR.

@quarkus-bot

This comment has been minimized.

@holly-cummins holly-cummins changed the base branch from main to development June 28, 2023 20:43
Copy link
Member

@rsvoboda rsvoboda left a comment

Choose a reason for hiding this comment

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

Pls use vvv instead of the current quarkus.opentelemetry.* entries, quarkus.opentelemetry.enabled=true imo doesn't need replacemt, just drop.

quarkus.otel.exporter.otlp.traces.endpoint=http://localhost:4317
quarkus.otel.exporter.otlp.traces.headers=Authorization=Bearer my_secret

@quarkus-bot
Copy link

quarkus-bot bot commented Jul 18, 2023

✔️ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

@rsvoboda rsvoboda merged commit 449c8e3 into quarkusio:development Jul 18, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing config files in opentelemetry-quickstart
5 participants