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) |
mcruzdev
changed the title
mcruzdev
changed the title
mcruzdev
force-pushed
the
convert-otel-duration-props
branch
2 times, most recently
from
614dc8d
to
7df8580
Compare
| } | ||
| } | ||
| } | ||
| return oTelConfigs; | ||
| } | ||
|
|
||
| private static String getConfigPropertyValue(String propertyName, ConfigValue smallRyeConfig) { |
There was a problem hiding this comment.
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.
It makes total sense, thank you!
| } | ||
| } | ||
| } | ||
| return oTelConfigs; | ||
| } | ||
|
|
||
| private static String getConfigPropertyValue(String propertyName, ConfigValue smallRyeConfig) { |
There was a problem hiding this comment.
We will need some tests with values that are currently accepted and others that are failing now.
There was a problem hiding this comment.
Hi @brunobat. do you mean inside integration-tests project, or here?
There was a problem hiding this comment.
they can be simple unit tests related to the OpenTelemetryUtil class.
There was a problem hiding this comment.
Perfect, I think that is ok now.
I changed some things:
getConfigPropertyValuetonormalizePropertyandconvertDurationtonormalizeDuration
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.
mcruzdev
force-pushed
the
convert-otel-duration-props
branch
from
a5da80b
to
84b2225
Compare
This comment has been minimized.
This comment has been minimized.
|
@mcruzdev you need to solve the failing tests |
mcruzdev
force-pushed
the
convert-otel-duration-props
branch
from
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? |
mcruzdev
force-pushed
the
convert-otel-duration-props
branch
from
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