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

fixing quoteservice metrics exporting #793

Merged

Conversation

brettmc
Copy link
Contributor

@brettmc brettmc commented Mar 15, 2023

Changes

metrics exporting was quietly failing due to misconfiguration. Update some packages to pick up the relevant bugfix, and use OTEL_EXPORTER_OTLP_ENDPOINT so that metrics, traces and eventually logs can use it.
In passing, enable some logging which makes future errors more obvious.

Merge Requirements

For new features contributions please make sure you have completed the following
essential items:

  • CHANGELOG.md updated to document new feature additions
  • Appropriate documentation updates in the docs

Maintainers will not merge until the above have been completed. If you're unsure
which docs need to be changed ping the
@open-telemetry/demo-approvers.

Fixes: #775

metrics exporting was quietly failing due to misconfiguration. Update some packages
to pick up the relevant bugfix, and use OTEL_EXPORTER_OTLP_ENDPOINT so that metrics,
traces and eventually logs can use it.
In passing, enable some logging which makes future errors more obvious.
@brettmc brettmc requested a review from a team March 15, 2023 09:29
@julianocosta89 julianocosta89 added the helm-update-required Requires an update to the Helm chart when released label Mar 15, 2023
@julianocosta89
Copy link
Member

Thanks for taking care of this @brettmc!
Could you add a CHANGELOG entry?

Copy link
Member

@julianocosta89 julianocosta89 left a comment

Choose a reason for hiding this comment

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

Tried to build it locally and started getting the following msg in the logs:

[2023-03-15T10:50:43.962596+00:00] otel-php.WARNING: tracestate discarded, invalid member:  {"source":"OpenTelemetry\\API\\Trace\\TraceState"} []

Is that expected?

@brettmc
Copy link
Contributor Author

brettmc commented Mar 15, 2023

@julianocosta89 no it's not expected...but I did turn on some logging so it's possible that it's a problem that's always been there but not noticed. I didn't see this when just calling the service directly (which would make sense, there is no distributed tracing).

To replicate, do I just bring up everything and watch it run?

CHANGELOG.md Outdated Show resolved Hide resolved
@julianocosta89
Copy link
Member

@julianocosta89 no it's not expected...but I did turn on some logging so it's possible that it's a problem that's always been there but not noticed. I didn't see this when just calling the service directly (which would make sense, there is no distributed tracing).

To replicate, do I just bring up everything and watch it run?

You can run:

docker compose build quoteservice --no-cache

docker compose up

Once everything is running, you can get the logs from quoteservice:

docker logs quoteservice

Co-authored-by: Juliano Costa <julianocosta89@outlook.com>
@brettmc
Copy link
Contributor Author

brettmc commented Mar 16, 2023

upstream bugfix: open-telemetry/opentelemetry-php#936

@brettmc
Copy link
Contributor Author

brettmc commented Mar 16, 2023

ok, patched in a new version of one of our packages, the warning message is gone, and we have one less bug in our code. I think this is ready now.

Copy link
Member

@julianocosta89 julianocosta89 left a comment

Choose a reason for hiding this comment

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

Awesome!

@julianocosta89 julianocosta89 merged commit 3680be7 into open-telemetry:main Mar 17, 2023
mat-rumian pushed a commit to SumoLogic/opentelemetry-demo that referenced this pull request Mar 20, 2023
* fixing quoteservice metrics exporting
metrics exporting was quietly failing due to misconfiguration. Update some packages
to pick up the relevant bugfix, and use OTEL_EXPORTER_OTLP_ENDPOINT so that metrics,
traces and eventually logs can use it.
In passing, enable some logging which makes future errors more obvious.

* changelog

* Update CHANGELOG.md

Co-authored-by: Juliano Costa <julianocosta89@outlook.com>

* fixing changelog link

* update to latest api with warning fixed

---------

Co-authored-by: Juliano Costa <julianocosta89@outlook.com>
@puckpuck puckpuck added the v1.4 required for 1.4 release label Mar 23, 2023
juliangiuca pushed a commit to juliangiuca/opentelemetry-demo that referenced this pull request Apr 12, 2023
* fixing quoteservice metrics exporting
metrics exporting was quietly failing due to misconfiguration. Update some packages
to pick up the relevant bugfix, and use OTEL_EXPORTER_OTLP_ENDPOINT so that metrics,
traces and eventually logs can use it.
In passing, enable some logging which makes future errors more obvious.

* changelog

* Update CHANGELOG.md

Co-authored-by: Juliano Costa <julianocosta89@outlook.com>

* fixing changelog link

* update to latest api with warning fixed

---------

Co-authored-by: Juliano Costa <julianocosta89@outlook.com>
jmichalak9 pushed a commit to jmichalak9/opentelemetry-demo that referenced this pull request Mar 22, 2024
* fixing quoteservice metrics exporting
metrics exporting was quietly failing due to misconfiguration. Update some packages
to pick up the relevant bugfix, and use OTEL_EXPORTER_OTLP_ENDPOINT so that metrics,
traces and eventually logs can use it.
In passing, enable some logging which makes future errors more obvious.

* changelog

* Update CHANGELOG.md

Co-authored-by: Juliano Costa <julianocosta89@outlook.com>

* fixing changelog link

* update to latest api with warning fixed

---------

Co-authored-by: Juliano Costa <julianocosta89@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
helm-update-required Requires an update to the Helm chart when released v1.4 required for 1.4 release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[shipping] reqwest spans taking a long duration
4 participants