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 gateway otlp config #10529

Merged
merged 2 commits into from
Feb 4, 2025
Merged

Fix gateway otlp config #10529

merged 2 commits into from
Feb 4, 2025

Conversation

flomonster
Copy link
Contributor

@flomonster flomonster commented Jan 27, 2025

  • service_name does not work (we could delete it and use the default env var)
  • figment::merge seems to work the other way around
  • We should move OTEL_SERVICE_NAME to docker files for all services

@codecov-commenter
Copy link

codecov-commenter commented Jan 27, 2025

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.88%. Comparing base (5d6174c) to head (ad7971d).
Report is 4 commits behind head on dev.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev   #10529      +/-   ##
==========================================
- Coverage   81.88%   81.88%   -0.01%     
==========================================
  Files        1078     1078              
  Lines      107139   107139              
  Branches      724      724              
==========================================
- Hits        87731    87728       -3     
- Misses      19368    19371       +3     
  Partials       40       40              
Flag Coverage Δ
editoast 74.17% <ø> (-0.01%) ⬇️
front 89.45% <ø> (ø)
gateway 2.18% <ø> (ø)
osrdyne 3.28% <ø> (ø)
railjson_generator 87.50% <ø> (ø)
tests 88.14% <ø> (ø)

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@github-actions github-actions bot added area:editoast Work on Editoast Service area:osrdyne labels Jan 27, 2025
@woshilapin woshilapin requested a review from ElysaSrc January 27, 2025 16:27
@woshilapin
Copy link
Contributor

woshilapin commented Jan 27, 2025

I've removed the configuration options for service name in gateway and osrdyne: let's use the standard OTEL_SERVICE_NAME environment variable for that. I've then set up that variable directly in the Dockerfile of the three Rust components.

Finally, I've remove the enable in gateway's configuration as it's a leftover from b154bad.

I still need to test more this figment::merge to understand what is going on since it looks really weird.

@flomonster flomonster marked this pull request as ready for review February 4, 2025 14:57
@flomonster flomonster requested review from a team as code owners February 4, 2025 14:57
@flomonster flomonster force-pushed the fam/fix-otlp-gateway branch from 9457bc9 to 82267bc Compare February 4, 2025 14:57
Copy link
Contributor

@bloussou bloussou left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

flomonster and others added 2 commits February 4, 2025 16:28
Signed-off-by: Florian Amsallem <florian.amsallem@gmail.com>
…ice name

Let's use the standard `OTEL_SERVICE_NAME`. Let's also put that configuration
variable directly in the `Dockerfile`.

Finally, remove the `enable` which is a leftover from b154bad.

Signed-off-by: Jean SIMARD <woshilapin@tuziwo.info>
@flomonster flomonster force-pushed the fam/fix-otlp-gateway branch from 82267bc to ad7971d Compare February 4, 2025 15:29
@flomonster flomonster added this pull request to the merge queue Feb 4, 2025
Merged via the queue into dev with commit 63347a3 Feb 4, 2025
27 checks passed
@flomonster flomonster deleted the fam/fix-otlp-gateway branch February 4, 2025 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants