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

Improve key/value pairs in headers #72

Closed
marcalff opened this issue Feb 8, 2024 · 4 comments · Fixed by #115
Closed

Improve key/value pairs in headers #72

marcalff opened this issue Feb 8, 2024 · 4 comments · Fixed by #115

Comments

@marcalff
Copy link
Member

marcalff commented Feb 8, 2024

In the Headers node, key/values are represented like this currently:

headers:
  key_a: value_a
  key_b: value_b
  api-key: 1234

The key name appear as a node name, on the left hand side (LHS).

Issue 1: This most likely adds some constraints on the name itself, due to yaml.

Can key names that include special characters be represented ?

Issue 2: The key name can not be substituted with environment variables.

This is a concern for keys like api-key, where the exact key name to use can be vendor dependent: it is highly desirable to abstract this away.

Proposed solution:

headers:
  - key:
    name: key_a
    value: value_a
  - key:
    name: "key_b#from%Hell$$" <-- fixes issue 1
    value: value_b
  - key:
    name: ${VENDOR_AUTH_KEY_NAME} <-- fixes issue 2
    value: ${VENDOR_AUTH_KEY_TOKEN}
@marcalff
Copy link
Member Author

marcalff commented Feb 9, 2024

This model change is a pre-requisite to later support secrets, as discussed in #69 (comment)

@jack-berg
Copy link
Member

Issue 1: This most likely adds some constraints on the name itself, due to yaml.

I'm pretty sure yaml allows keys to be contain character besides control characters. Can include the key in double quotes and escape if needed. The key_b#from%Hell$$ doesn't appear to need to be contained in double quotes to be valid.

Issue 2: The key name can not be substituted with environment variables.

Hmm.. I want to be careful with this because this argument could potentially be applicable in a lot of places, and encourage us to module using object arrays instead of maps, which is a much less intuitive user experience.

This is a concern for keys like api-key, where the exact key name to use can be vendor dependent: it is highly desirable to abstract this away.

This intersects with this conversation. If we choose to flatten header configure to a comma separated list of key value pairs, then its perfectly possible to do this:

headers: "key_a=value_a,${VENDOR_AUTH_KEY_NAME}=${VENDOR_AUTH_KEY_TOKEN}"

@jack-berg
Copy link
Member

Moved to "TODO" column. I think we need to make a decision on this one way or another before stabilizing. With #106 merged, we can now reasonably add an alternative way to specify OTLP exporter headers to support OTEL_EXPORTER_OTLP_HEADERS and this use case:

tracer_provider:
  processors:
    - batch:
        exporter:
          otlp:
            headers:
              key1: value1
           headers_list: "${VENDOR_AUTH_KEY_NAME}:${VENDOR_AUTH_KEY_TOKEN}" 

But we still might consider accepting this as a preferred way to represent general mappings in config. For one thing, it goes a long way to ensuring that all property keys are able to follow our snake case naming convention. We may want to formalize guidance against having user provided, unbound property keys - even if they are possible to express in YAML.

Note that kubernetes manifest use something similar to this issue's suggested syntax to express env vars.

@jack-berg
Copy link
Member

I've opened a #115 with to switch to this recommendation.

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

Successfully merging a pull request may close this issue.

2 participants