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

Make error message about non-escaped ENV variables clearer #2376

Closed
cincodenada opened this issue Jan 14, 2022 · 1 comment · Fixed by #3044
Closed

Make error message about non-escaped ENV variables clearer #2376

cincodenada opened this issue Jan 14, 2022 · 1 comment · Fixed by #3044

Comments

@cincodenada
Copy link

I ran into the issue of having to escape the ENV variables (particularly relevant for auth headers that have spaces) recently, and the error message was only somewhat helpful. I read up on some history here (#2248, #2103, #2010) and I think the error message should note that the ENV variables need to be URL escaped, preferably directly referencing the OpenTelemetry Protocol Exporter specification that this project is following.

I think it's particularly useful here since the parsing here is a change from 1.5.0, so if folks are upgrading down the road, their existing ENV variables may stop working, and it'd be nice to help point them to what they need to change.

Looking into the code in re.py, I think the method is too broadly named - it should probably be something like parse_env_headers to avoid confusion so it doesn't seem like a general-purpose header parser to folks in the future. One could also regex

Is your feature request related to a problem?
Upgrading breaks OTEL configuration ENV strings, and the error message is not super helpful

Describe the solution you'd like
I'd like a more helpful error message that tells me that my ENV variables need to be URL encoded now

Describe alternatives you've considered
You could leave it as is, it'd just confuse people. Alternately, you could be backwards compatible, and if there are env variables that don't parse, try parsing them as unquoted env variables.

Additional context
I have some very concrete suggestions in #2375, but I guess don't look at them if taking inspiration from them is gonna cause legal troubles. Again, you can do whatever the heck you want with them as far as I'm concerned, but I'm not gonna sign a CLA just to update an error message and try to make code I am unlikely to work with clearer.

@srikanthccv
Copy link
Member

PR #2375 attempts to address this; Can copy the changes by someone willing to sign or already signed the CLA.

ocelotl added a commit to ocelotl/opentelemetry-python that referenced this issue Nov 17, 2022
ocelotl added a commit to ocelotl/opentelemetry-python that referenced this issue Nov 17, 2022
ocelotl added a commit to ocelotl/opentelemetry-python that referenced this issue Nov 18, 2022
Also, rename the function to make it clear that this parsing is specific
to headers provided via ENV variables.

Fixes open-telemetry#2376
ocelotl added a commit to ocelotl/opentelemetry-python that referenced this issue Nov 29, 2022
Also, rename the function to make it clear that this parsing is specific
to headers provided via ENV variables.

Fixes open-telemetry#2376
ocelotl added a commit to ocelotl/opentelemetry-python that referenced this issue Nov 30, 2022
Also, rename the function to make it clear that this parsing is specific
to headers provided via ENV variables.

Fixes open-telemetry#2376
ocelotl added a commit that referenced this issue Nov 30, 2022
* Add a more informative error message when parsing ENV headers

Also, rename the function to make it clear that this parsing is specific
to headers provided via ENV variables.

Fixes #2376

* Use parse_env_headers in metrics and logs as well

* Fix lint

* Fix mypy
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants