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

Refactor trace header support #59

Merged
merged 2 commits into from
Sep 12, 2024
Merged

Refactor trace header support #59

merged 2 commits into from
Sep 12, 2024

Conversation

costela
Copy link
Contributor

@costela costela commented Aug 12, 2024

Following up from the discussion in #57 (comment)

This was referenced Aug 12, 2024
Closed
Closed
riandyrn
riandyrn requested changes Aug 19, 2024
View reviewed changes
Copy link
Owner

@riandyrn riandyrn left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Hello, @Costella

Thanks for your new PR, and I'm sorry for the late review.

However, I noticed in this PR, you are trying to do several things:

  1. Implement WithTraceResponseHeaders() option & deprecate WithTraceIDResponseHeader.
  2. Refactor config by making all the properties private.
  3. Refactor sdk_test.go by adding t.Helper() to several test cases.

While it is tempting to implement all of these in one go, let's only focus on implementing the first objective. This will make the review much easier and ensure we won't run into unintended consequences.

cc: @ilhamsyahids

@costela costela force-pushed the only-add-response-header-if-trace-was-sampled branch from cc07705 to 489a49a Compare August 19, 2024 19:57
@costela costela force-pushed the only-add-response-header-if-trace-was-sampled branch from 489a49a to 2cf1c02 Compare August 19, 2024 20:04
@costela
Copy link
Contributor Author

costela commented Aug 19, 2024

@riandyrn yes, that's reasonable! Just removed most the rest of the changes. Will split them into another PR for consideration.
Kept the two t.Helper instance that are actually called in the new test, since they are arguably part of the change. WDYT?

@costela costela requested a review from riandyrn August 19, 2024 20:05
@costela costela mentioned this pull request Aug 19, 2024
Merged
@costela
Copy link
Contributor Author

costela commented Sep 9, 2024

Just a friendly ping, in case anyone happens to have some time for a review this week 😇

@costela
Copy link
Contributor Author

costela commented Sep 11, 2024

the pipeline failure seems unrelated? 🤔

ilhamsyahids
ilhamsyahids approved these changes Sep 12, 2024
View reviewed changes
Copy link
Collaborator

@ilhamsyahids ilhamsyahids left a comment

Choose a reason for hiding this comment

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

LGTM, thanks a lot @costela! 🔥🚀

@ilhamsyahids ilhamsyahids changed the base branch from master to feat/trace-sampled September 12, 2024 11:44
@ilhamsyahids
Copy link
Collaborator

Not sure about the pipeline, let me try on different branch

@ilhamsyahids ilhamsyahids merged commit c866ef8 into riandyrn:feat/trace-sampled Sep 12, 2024
10 of 11 checks passed
@ilhamsyahids ilhamsyahids mentioned this pull request Sep 12, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ilhamsyahids ilhamsyahids ilhamsyahids approved these changes

@riandyrn riandyrn Awaiting requested review from riandyrn

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@costela @ilhamsyahids @riandyrn