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

Extract OTEL parsing to configMap to allow consistent handling of env #719

Merged
merged 3 commits into from
Nov 18, 2024

Conversation

DiamondJoseph
Copy link
Contributor

This handling does not require that string parsing is done on the extraEnvVars, and so resolves the need for them to be a string re: #582

Copy link

codecov bot commented Nov 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.27%. Comparing base (fa20204) to head (3771e51).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #719   +/-   ##
=======================================
  Coverage   92.27%   92.27%           
=======================================
  Files          35       35           
  Lines        1800     1800           
=======================================
  Hits         1661     1661           
  Misses        139      139           

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

Copy link
Contributor

@callumforrester callumforrester left a comment

Choose a reason for hiding this comment

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

I suggest @keithralphs take a look too.

I'm happy with this change. It does, however, bring to mind why I initially suggested aligning the OTEL configuration with the rest of the BlueAPI configuration (loaded from YAML, etc.). While it was considered too complex due to the internals of the OTEL library, the fact that we've had to put special handling in here makes me wonder if it's worth revisiting this approach.

Maybe make an issue?

@@ -91,10 +85,11 @@ extraEnvVars: |

tracing:
otlp:
export_enabled: false
enabled: false
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this doing anything?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just me being a hater on how clunky the key name is.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I agree with that, but is that key actually used anywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

{{ if .Values.tracing.otlp.enabled | default false }}

It is the condition that enables filling the ConfigMap.

@keithralphs
Copy link
Contributor

need to resolve the port and protocol spec ot it won't work

@DiamondJoseph
Copy link
Contributor Author

@keithralphs fixed both, and have tested that the config map made locally is formatted correctly.

@DiamondJoseph DiamondJoseph merged commit a4e881d into main Nov 18, 2024
29 checks passed
@DiamondJoseph DiamondJoseph deleted the otel-config branch November 18, 2024 10:38
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.

3 participants