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.

Sign up for GitHub

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

Jump to bottom

[FSSDK-9862] Add OpenTelemetry Tracing support #385

Merged
merged 21 commits into from
Jan 18, 2024
Merged

[FSSDK-9862] Add OpenTelemetry Tracing support #385

merged 21 commits into from
Jan 18, 2024

Conversation

pulak-opti
Copy link
Contributor

@pulak-opti pulak-opti commented Jan 12, 2024 •
edited
Loading

Summary

  • Added OpenTelemetry tracing support
  • Removed deprecated io/ioutil package from example code, used os package instead

Ticket

@pulak-opti pulak-opti marked this pull request as ready for review January 16, 2024 16:51
@pulak-opti pulak-opti requested a review from a team as a code owner January 16, 2024 16:51
@pulak-opti pulak-opti changed the title [FSSDK-9862] Implementation Open Telemetry Traces [FSSDK-9862] Add OpenTelemetry Tracing support Jan 16, 2024
mikechu-optimizely
mikechu-optimizely approved these changes Jan 16, 2024
View reviewed changes
Copy link
Contributor

@mikechu-optimizely mikechu-optimizely left a comment

Choose a reason for hiding this comment

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

Lots of func sig changes but LGTM

@mikechu-optimizely
Copy link
Contributor

mikechu-optimizely commented Jan 16, 2024 •
edited
Loading

Lots of func sig changes but LGTM

...but also good to see some dependency removal (ioutil) from the example, version bumps, along with the OT additions. May want to list the objectives in the PR description for external future ref.

mikechu-optimizely
mikechu-optimizely approved these changes Jan 17, 2024
View reviewed changes
Copy link
Contributor

@mikechu-optimizely mikechu-optimizely left a comment

Choose a reason for hiding this comment

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

I've offered a few suggestions, but none are showstoppers in my opinion, so Approved.

I do like the WithTraceContext() concept 👍

pkg/client/client.go Outdated Show resolved Hide resolved
pkg/client/client.go Outdated Show resolved Hide resolved
pkg/client/client.go Outdated Show resolved Hide resolved
pkg/client/client_test.go Show resolved Hide resolved
pkg/client/optimizely_user_context.go Show resolved Hide resolved
pkg/client/optimizely_user_context_odp_decide_test.go Outdated Show resolved Hide resolved
jaeopt
jaeopt requested changes Jan 17, 2024
View reviewed changes
Copy link

@jaeopt jaeopt left a comment

Choose a reason for hiding this comment

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

Looks great! I love the solution. A clarification question.

jaeopt
jaeopt approved these changes Jan 18, 2024
View reviewed changes
Copy link

@jaeopt jaeopt left a comment

Choose a reason for hiding this comment

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

LGTM

@pulak-opti pulak-opti merged commit 4adb1af into master Jan 18, 2024
14 checks passed
@pulak-opti pulak-opti deleted the pulak/add-sdk-telemetry branch January 18, 2024 17:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mikechu-optimizely mikechu-optimizely mikechu-optimizely approved these changes

@jaeopt jaeopt jaeopt approved these changes

Assignees

@pulak-opti pulak-opti

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pulak-opti @mikechu-optimizely @jaeopt