[cmd/telemetrygen] remove reliance on JSON unmarshalling for status code string #26985
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…mmand - strings were not valid JSON and would therefore cause the JSON Unmarshal to return an error - each status code is now captured explicitly in a switch case statement - mixed case is now supported when passing it as a string also - `TestSpanStatuses` test was updated to run as sub-tests
jmsnll
commented
Sep 18, 2023
fatsheep9146
requested changes
Sep 19, 2023
There was a problem hiding this comment.
Thanks for contribution!
I think we'd better merge this after #26668 is merged, in order to double check is pr is valid.
mx-psi
approved these changes
Sep 19, 2023
|
I merged #26668, @fatsheep9146 can you take another look? |
Thanks, I just merged the main to see if the check is passed @mx-psi After all checks are passed, I will approve the pr. |
fatsheep9146
approved these changes
Sep 25, 2023
fatsheep9146
added
the
ready to merge
All related checks are passed, I approved the pr @mx-psi |
jmsnll
added a commit
to jmsnll/opentelemetry-collector-contrib
that referenced
this pull request
Nov 12, 2023
…ode string (open-telemetry#26985) Removes the use of JSON Unmarshalling when determining the provided trace status code. The existing implementation allowed both status code and status strings to be set using the command line flag `--status-code` without issue however it's not obvious that the default command line value must be a JSON string as encountered in open-telemetry#25906 I've replaced the existing code with a switch-case statement which is more explicit. Additionally, the code now checks the lowercase version of the provided string, enabling support for mixed-case inputs such as `--status-code unset` and `--status-code Unset` flags. I'm undecided if this is actually better or required since we now understand the original cause of the issue. Example usage is now: ``` telemetrygen traces --otlp-insecure --traces 1 --status-code 0 # unset telemetrygen traces --otlp-insecure --traces 1 --status-code 1 # error telemetrygen traces --otlp-insecure --traces 1 --status-code 2 # ok telemetrygen traces --otlp-insecure --traces 1 --status-code unset telemetrygen traces --otlp-insecure --traces 1 --status-code error telemetrygen traces --otlp-insecure --traces 1 --status-code ok ``` Fixes open-telemetry#25906 Added additional test cases and implemented sub-tests as the tests were quitting after the first falsified case. --------- Co-authored-by: Ziqi Zhao <[email protected]> Co-authored-by: Pablo Baeyens <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description:
Removes the use of JSON Unmarshalling when determining the provided trace status code.
The existing implementation allowed both status code and status strings to be set using the command line flag
--status-codewithout issue however it's not obvious that the default command line value must be a JSON string as encountered in #25906I've replaced the existing code with a switch-case statement which is more explicit. Additionally, the code now checks the lowercase version of the provided string, enabling support for mixed-case inputs such as
--status-code unsetand--status-code Unsetflags.I'm undecided if this is actually better or required since we now understand the original cause of the issue.
Example usage is now:
Link to tracking Issue: Fixes #25906
Testing:
Added additional test cases and implemented sub-tests as the tests were quitting after the first falsified case.
Documentation: No updates to documentation.