-
Notifications
You must be signed in to change notification settings - Fork 896
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
[configuration] Force type when doing environment variable substitution #4281
Comments
Seems like this should be transferred to |
Filed open-telemetry/opentelemetry-collector-contrib#36183 though I feel like this is not a one-processor-specific problem. |
hi @flyfire2002, we don't think this is a spec issue since the spec doesn't say anything about collector processors |
I think this is indeed a spec issue as the issue is not with collector processors but with how the collector configuration system handles environment variable substitution. The expressed intent there is for the behavior to "support the exact same syntax as the Configuration WG." Here collector processors were only used as the medium for conveying a description of the issue. If the behavior the Configuration WG expects is for an environment variable containing |
@open-telemetry/configuration-approvers can you review @Aneurysm9's #4281 (comment)? thanks! |
There are two mechanisms for this. First, if you want to ensure that an ambiguous value like
You can see an example of this in the spec here. Second, you can use YAML tags to coerce the parser to interpret the value as a specific type:
|
This does not work. Perhaps this is a defect in the collector's handling of substitutions. I'm also not sure the linked example is illustrating this as I can't tell whether To be more concrete, here's an example of a collector configuration chunk that has the issue: ACCOUNT_ID=012345678912 attributes/add_labels:
actions:
- action: insert
key: account_id
value: ${env:ACCOUNT_ID} # evaluates to `12345678912` Wrapping the substitution in quotes at this layer of processing has no effect as the input has already been cast as an integer. IOW, this configuration produced the same result, namely a truncated account ID: attributes/add_labels:
actions:
- action: insert
key: account_id
value: "${env:ACCOUNT_ID}" # evaluates to `12345678912` Wrapping the value in the environment variable in quotes such that they were part of the value prevented integer conversion and truncation, but then resulted in undesired literal quotes in the attribute value: ACCOUNT_ID="\"012345678912\"" attributes/add_labels:
actions:
- action: insert
key: account_id
value: ${env:ACCOUNT_ID} # evaluates to `"012345678912"` Ultimately, in this particular situation we were able to work around the issue by reading the environment variable with the quotes into a temporary attribute, extracting the desired digits from that temporary attribute, and then deleting the temporary attribute. This is obviously less than ideal and simply not an option in some situations. attributes/add_labels:
actions:
- action: insert
key: tmp_account_id
value: ${env:ACCOUNT_ID}
- action: extract
key: tmp_account_id
pattern: (?<account_id>\d{12})
- action: delete
key: tmp_account_id
Perhaps this is a collector defect, but I tried this: ACCOUNT_ID="!!str 012345678912" and the injected attribute value was |
The string value of the environment variable is substituted in place of the env var reference as is, without coercion. Then, the YAML parser is response for taking the resulting value and resolving the tag type. For example:
This behavior should produce the results you're looking for. Assuming
I can't say whether this is intentional design of collector or a defect, but I can say that this is not the intended or actual behavior of declarative config.
I would have expected you to do
That should result in a value resolved to type string, tag URI tag:yaml.org,2002:str. |
Yes. That's exactly the behavior I would expect as well. It is not the behavior that was encountered nor is it the documented expected behavior from the collector SIG where they believe they are aligning with this specification.
This is worrying, then, as the collector SIG's docs say:
If the collector SIG doesn't correctly interpret this spec, can anyone?
That doesn't work because the |
@mx-psi can you or someone else from @open-telemetry/collector-maintainers take a look at this and confirm where the issue lies? |
Here's a unit test in the java implementation showing how it resolves types when env var substitution is used, demonstrating with and without quotes: https://github.com/open-telemetry/opentelemetry-java/blob/13e2b8c4dfeba09f361b47986febbd792ab1ae60/sdk-extensions/incubator/src/test/java/io/opentelemetry/sdk/extension/incubator/fileconfig/FileConfigurationParseTest.java#L591-L659
Oh I see. It depends on how the implementation substitutes the value and when the type is resolved. In the java implementation a reference, given I'd argue this is the correct implementation. It seems like the collector is trying to interpret the type of the env var value and injecting a string representation of the interpreted type. If this is indeed the case, I'm not sure what the advantage of this could be. |
Oh, it's |
What are you trying to achieve?
When substituting env variables like "00001" (as a string) (e.g. into an attribute), the type of the produced value is automatically Int. I want to be able to force it to type Str at substitution. Note, currently with
attribute: convert
it is done after the value is already substituted as an Int, so in this case it will be doing Int(1) => Str(1), which is still not the same as Str(00001).** Context/Example **
With config
When the env var id = "00001", the value of the attribute "some_id" will be Int(1).
If we do a
afterwards, the value is Str(1) which is still different from "00001" / Str(00001).
The text was updated successfully, but these errors were encountered: