-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Convert duration properties before send to OTel #33916
Conversation
Thanks for your pull request! The title of your pull request does not follow our editorial rules. Could you have a look?
This message is automatically generated by a bot. |
/cc @brunobat (opentelemetry), @radcortez (opentelemetry) |
614dc8d
to
7df8580
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for the PR.
I requested some changes.
Also. Please build the extension code before committing... It will be automatically reformatted if it doesn't pass the guidelines.
} | ||
} | ||
} | ||
return oTelConfigs; | ||
} | ||
|
||
private static String getConfigPropertyValue(String propertyName, ConfigValue smallRyeConfig) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can move this static method to util as well. It will contain the needed logic in one spot.
Calling it something like normalizeDuration would also help.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes total sense, thank you!
} | ||
} | ||
} | ||
return oTelConfigs; | ||
} | ||
|
||
private static String getConfigPropertyValue(String propertyName, ConfigValue smallRyeConfig) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will need some tests with values that are currently accepted and others that are failing now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @brunobat. do you mean inside integration-tests
project, or here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
they can be simple unit tests related to the OpenTelemetryUtil
class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect, I think that is ok now.
I changed some things:
getConfigPropertyValue
tonormalizeProperty
andconvertDuration
tonormalizeDuration
I think that is more clear now, if is necessary to normalize another property we add it to the map that I renamed to OTEL_PROPERTIES_NORMALIZED
. What do you think?
This comment has been minimized.
This comment has been minimized.
Looks ok @mcruzdev. I just need you to squash everything into 1 commit, please. |
This comment has been minimized.
This comment has been minimized.
a5da80b
to
84b2225
Compare
This comment has been minimized.
This comment has been minimized.
@mcruzdev you need to solve the failing tests |
5a47a46
to
58e6967
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
What's the status of this one? |
Hi @geoand, how are you? |
Thanks for the update. What's the problem with the tests? |
58e6967
to
910f56b
Compare
Hi @geoand thank you! |
You can debug anyone of the failing test like so:
and then attach a remote debugger |
@mcruzdev where you perhaps able to figure out what the problem with the PR is? |
Failing Jobs - Building 910f56b
Full information is available in the Build summary check run. Failures⚙️ JVM Tests - JDK 11 #- Failing: extensions/opentelemetry/deployment
! Skipped: extensions/micrometer-registry-prometheus/deployment extensions/micrometer/deployment extensions/scheduler/deployment and 26 more 📦 extensions/opentelemetry/deployment✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ⚙️ JVM Tests - JDK 17 #- Failing: extensions/opentelemetry/deployment
! Skipped: extensions/micrometer-registry-prometheus/deployment extensions/micrometer/deployment extensions/scheduler/deployment and 26 more 📦 extensions/opentelemetry/deployment✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ⚙️ JVM Tests - JDK 17 Windows #- Failing: extensions/opentelemetry/deployment
! Skipped: extensions/micrometer-registry-prometheus/deployment extensions/micrometer/deployment extensions/scheduler/deployment and 26 more 📦 extensions/opentelemetry/deployment✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ⚙️ JVM Tests - JDK 19 #- Failing: extensions/opentelemetry/deployment
! Skipped: extensions/micrometer-registry-prometheus/deployment extensions/micrometer/deployment extensions/scheduler/deployment and 26 more 📦 extensions/opentelemetry/deployment✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ⚙️ Native Tests - Misc4 #- Failing: integration-tests/opentelemetry
📦 integration-tests/opentelemetry✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ |
@mcruzdev do you need help with the tests? |
Fixes #33381