Skip to content

feat: add telemetry helper utils #1346

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

Merged
merged 7 commits into from
May 13, 2025
Merged

Conversation

liran2000
Copy link
Member

This PR

adds a method to core that returns a semantically valid flag evaluation event

Related Issues

Fixes #1327

Similar to GO PR.

@liran2000 liran2000 requested a review from a team as a code owner February 25, 2025 08:55
Signed-off-by: liran2000 <[email protected]>
Copy link

codecov bot commented Feb 25, 2025

Codecov Report

Attention: Patch coverage is 96.77419% with 1 line in your changes missing coverage. Please review.

Project coverage is 93.35%. Comparing base (58454b4) to head (dbc41b7).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
src/main/java/dev/openfeature/sdk/Telemetry.java 96.66% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #1346      +/-   ##
============================================
+ Coverage     92.81%   93.35%   +0.53%     
- Complexity      469      480      +11     
============================================
  Files            43       45       +2     
  Lines          1127     1158      +31     
  Branches         91       99       +8     
============================================
+ Hits           1046     1081      +35     
+ Misses           53       48       -5     
- Partials         28       29       +1     
Flag Coverage Δ
unittests 93.35% <96.77%> (+0.53%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@liran2000 liran2000 requested a review from beeme1mr February 25, 2025 09:56
Copy link
Member

@aepfli aepfli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you.

Copy link

* @return an EvaluationEvent populated with telemetry data
*/
public static EvaluationEvent createEvaluationEvent(
HookContext<?> hookContext, ProviderEvaluation<?> providerEvaluation) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we want to use FlagEvaluationDetails here instead of the ProviderEvaluation, as pointed out here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, good catch. The finally hook was updated in this PR.

Copy link
Member

@beeme1mr beeme1mr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The OTel folks requested that we make a couple of changes in the OTel semcon. Let's wait to merge this until those issues have been resolved.

https://cloud-native.slack.com/archives/C07AES1JN56/p1741799036596509

@toddbaert
Copy link
Member

The OTel folks requested that we make a couple of changes in the OTel semcon. Let's wait to merge this until those issues have been resolved.

https://cloud-native.slack.com/archives/C07AES1JN56/p1741799036596509

@beeme1mr can we revive this PR now?

cc @liran2000

@toddbaert toddbaert requested review from chrfwow and beeme1mr March 25, 2025 16:44
@chrfwow
Copy link
Contributor

chrfwow commented May 9, 2025

@liran2000 is there any progress on this PR or do you plan to wait for the OTel PR to be merged? I think we can proceed under the assumption that the PR will be merged as it is currently.

@liran2000
Copy link
Member Author

@liran2000 is there any progress on this PR or do you plan to wait for the OTel PR to be merged? I think we can proceed under the assumption that the PR will be merged as it is currently.

Hi, waiting for @beeme1mr according to his comment.

The semconv has changed, and some attributes have been
renamed. Furthermore, the body usage is deprecated
and should be part of the attributes.

see: open-telemetry/semantic-conventions#1990
Signed-off-by: Simon Schrottner <[email protected]>
@aepfli aepfli requested a review from a team as a code owner May 13, 2025 06:34
Signed-off-by: Simon Schrottner <[email protected]>
@aepfli
Copy link
Member

aepfli commented May 13, 2025

Hey @liran2000, thank you for doing the heavy lifting with this pr. As the semconv has been merged, I adapted your code (renamings mainly) so we can get this merged. Thank you so much!

@aepfli aepfli dismissed beeme1mr’s stale review May 13, 2025 06:43

the reason for requested changes has been lifted, the semconv is merged, and we could adapt the pull request based on it

Copy link

@aepfli aepfli merged commit d0ae548 into open-feature:main May 13, 2025
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add OpenTelemetry-Compatible Telemetry Utility Function
5 participants