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 traces to ofrep endpoint #1593

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

aepfli
Copy link
Member

@aepfli aepfli commented Mar 14, 2025

This PR

our OFREP endpoint was not offering any kind of otel information, now we have traces
image

Related Issues

Relates: open-telemetry/opentelemetry-demo#2119

Notes

Follow-up Tasks

How to test

@aepfli aepfli requested a review from a team as a code owner March 14, 2025 10:28
@dosubot dosubot bot added the size:S This PR changes 10-29 lines, ignoring generated files. label Mar 14, 2025
Copy link

netlify bot commented Mar 14, 2025

Deploy Preview for polite-licorice-3db33c canceled.

Name Link
🔨 Latest commit 5fa98b8
🔍 Latest deploy log https://app.netlify.com/sites/polite-licorice-3db33c/deploys/67d46ffe5f97db00085bcb29

@aepfli aepfli force-pushed the feat/add_traces_for_ofrep branch from 6b44e01 to 77ea788 Compare March 14, 2025 10:38
@julianocosta89
Copy link

This is great!
Thank you @aepfli

Copy link

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

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

I have some limited knowledge about both OpenFeature and feature flagging semconv, please ignore my comments wherever it does not make any sense :)

Overall, I'm wondering this PR should actually add logs for the internal operations instead, only keeping the server span for handling the HTTP request.

Signed-off-by: Simon Schrottner <[email protected]>
@aepfli aepfli force-pushed the feat/add_traces_for_ofrep branch from 77ea788 to 981b927 Compare March 14, 2025 16:31
@dosubot dosubot bot added size:XS This PR changes 0-9 lines, ignoring generated files. and removed size:S This PR changes 10-29 lines, ignoring generated files. labels Mar 14, 2025
@aepfli
Copy link
Member Author

aepfli commented Mar 14, 2025

I have some limited knowledge about both OpenFeature and feature flagging semconv, please ignore my comments wherever it does not make any sense :)

Overall, I'm wondering this PR should actually add logs for the internal operations instead, only keeping the server span for handling the HTTP request.

thank you for the great insights, i will limit this pr now, to only offer spans, but not adding any kind of additional information. Thank you

@toddbaert toddbaert requested review from beeme1mr and toddbaert March 15, 2025 17:20
@toddbaert
Copy link
Member

I have some limited knowledge about both OpenFeature and feature flagging semconv, please ignore my comments wherever it does not make any sense :)
Overall, I'm wondering this PR should actually add logs for the internal operations instead, only keeping the server span for handling the HTTP request.

thank you for the great insights, i will limit this pr now, to only offer spans, but not adding any kind of additional information. Thank you

I'm fine with limiting this pr, but I think I agree with @pichlermarc that we should limit span use and add more events (logs) before we release these changes. I think changing the nature of our telemetry to much can cause thrashing for monitoring setups.

What do you think @aepfli cc @beeme1mr

@toddbaert toddbaert requested a review from thisthat March 15, 2025 17:24
@toddbaert
Copy link
Member

Thanks for working on this @aepfli .

What do you think about this? #1593 (comment)

@aepfli
Copy link
Member Author

aepfli commented Mar 15, 2025

Thanks for working on this @aepfli .

What do you think about this? #1593 (comment)

with this pr, we are now generating traces, beforehand there was nothing - so i think it is a good starting point, especially for the ofrep endpoint, which is often related with a span from another system anyways

@beeme1mr
Copy link
Member

beeme1mr commented Mar 15, 2025

I think this is the right approach. We can easily add spans and custom attributes later once the use case becomes clear. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size:XS This PR changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants