Skip to content
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

feat: add spec compliant otel hook #169

Merged
merged 3 commits into from
Dec 22, 2022

Conversation

thiyagu06
Copy link
Member

@thiyagu06 thiyagu06 commented Dec 18, 2022

This PR

added otel spec compliant hook.

Related Issues

Fixes #144

Notes

Follow-up Tasks

How to test

add the hook and see the result in Jaeger tracing UI.

openFeatureAPI.addHooks(new OpenTelemetryHook());

image

@thiyagu06 thiyagu06 changed the title update otel hook to be spec compliant feat:! update otel hook to be spec compliant Dec 18, 2022
@thiyagu06 thiyagu06 changed the title feat:! update otel hook to be spec compliant feat!: update otel hook to be spec compliant Dec 18, 2022
hooks/README.md Outdated Show resolved Hide resolved
@beeme1mr
Copy link
Member

Thanks @thiyagu06! We'll review this tomorrow but it looks great at first glance.

I noticed in the screenshot you attached that the variant is quoted. Is that expected?

@thiyagu06
Copy link
Member Author

Thanks @thiyagu06! We'll review this tomorrow but it looks great at first glance.

I noticed in the screenshot you attached that the variant is quoted. Is that expected?

yeah. Thanks for pointing that. the variant is double quoted because, I convert the value into json format.

The reason behind that is I'm not sure what kind of values use will be adding for the Object type. Is that okay if we could use toString method instead of using jackson serialise it.

@beeme1mr beeme1mr changed the title feat!: update otel hook to be spec compliant feat: update otel hook to be spec compliant Dec 22, 2022
@beeme1mr beeme1mr changed the title feat: update otel hook to be spec compliant feat: add spec compliant otel hook Dec 22, 2022
@toddbaert
Copy link
Member

toddbaert commented Dec 22, 2022

Thanks @thiyagu06! We'll review this tomorrow but it looks great at first glance.
I noticed in the screenshot you attached that the variant is quoted. Is that expected?

yeah. Thanks for pointing that. the variant is double quoted because, I convert the value into json format.

The reason behind that is I'm not sure what kind of values use will be adding for the Object type. Is that okay if we could use toString method instead of using jackson serialise it.

Yes, in fact, I think we should try to avoid dependencies such as jackson here. I think .toString() is fine.

Complex object values are represented by our dev.openfeature.sdk.Structure interfaces, which house attribute maps and can have their own toString implementation (though for the current implementations, the default works fine). I don't think we need to standardize on JSON here.

If in the future we decide to standardize the representation of these objects, we could implement one in the overridden toString.

@thiyagu06
Copy link
Member Author

Thanks @thiyagu06! We'll review this tomorrow but it looks great at first glance.
I noticed in the screenshot you attached that the variant is quoted. Is that expected?

yeah. Thanks for pointing that. the variant is double quoted because, I convert the value into json format.

The reason behind that is I'm not sure what kind of values use will be adding for the Object type. Is that okay if we could use toString method instead of using jackson serialise it.

Yes, in fact, I think we should try to avoid dependencies such as jackson here. I think .toString() is fine.

Complex object values are represented by our dev.openfeature.sdk.Structure interfaces, which house attribute maps and can have their own toString implementation (though for the current implementations, the default works fine). I don't think we need to standardize on JSON here.

If in the future we decide to standardize the representation of these objects, we could implement one in the overridden toString.

Yeah. I understand that. Fixed it

@toddbaert toddbaert self-requested a review December 22, 2022 14:39
Copy link
Member

@toddbaert toddbaert left a comment

Choose a reason for hiding this comment

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

My only real concern is: https://github.com/open-feature/java-sdk-contrib/pull/169/files#r1055525423

Once this is addressed it looks like it can be merged. Thanks a lot, especially for even giving this a test in Jaeger.

Copy link
Member

@toddbaert toddbaert left a comment

Choose a reason for hiding this comment

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

Looks good to me, thank you!

Signed-off-by: thiyagu06 <thiyagu103@gmail.com>
…n info in README.md

Signed-off-by: thiyagu06 <thiyagu103@gmail.com>
Signed-off-by: thiyagu06 <thiyagu103@gmail.com>
@toddbaert toddbaert merged commit 55a2ac2 into open-feature:main Dec 22, 2022
@github-actions github-actions bot mentioned this pull request Dec 22, 2022
@thiyagu06 thiyagu06 deleted the otel-spec-compliant branch December 23, 2022 03:19
DBlanchard88 pushed a commit to DBlanchard88/java-sdk-contrib that referenced this pull request Apr 29, 2024
…t to 44c30b3 (open-feature#169)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
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 OTel Hook
3 participants