Fix analytics.event override #2409
Merged
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
PROFeNoM
added
🐛 bug
|
I confirm that the system tests would pass with this change (with respect to the alex/otel-api branch) |
pierotibou
reviewed
Dec 12, 2023
ext/serializer.c
Outdated
| @@ -1202,6 +1202,11 @@ void ddtrace_shutdown_span_sampling_limiter(void) { | |||
| zend_hash_destroy(&dd_span_sampling_limiters); | |||
| } | |||
|
|
|||
| static zend_always_inline double ddtrace_convert_string_to_double(zend_string *str) { | |||
There was a problem hiding this comment.
There's no possibility for that use case to be something else than true? I ask because we do something more comprehensive in .NET
There was a problem hiding this comment.
Indeed. I re-read the RFC, and it must match the same behavior as strconv.ParseBool. I'll change this as soon as I can.
Note: It's not tested in the system tests right now. I will add them.
There was a problem hiding this comment.
I made a system tests PR. Let's wait on it to assess the correct behavior to go with 😃
There was a problem hiding this comment.
Thanks a lot for the thorough follow up!
PROFeNoM
force-pushed
the
alex/fix/otel-analytics-event
branch
from
4bdc524 to
9b00079
Compare
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
A system-test testing the behavior of
analytics.eventtag override was added with DataDog/system-tests#1818. The current implementation would fail :)This PR simply explicitly checks for 'true' (not case sensitive) to return
1.0, and anything else would yield0.0I set it as
do-not-mergeas I need the pipeline to finish to test it properly, but the local tests otherwise pass.Reviewer checklist