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

contrib/internal/httputil: add HTTP host tag #648

Merged
merged 9 commits into from May 15, 2020
Merged

contrib/internal/httputil: add HTTP host tag #648

merged 9 commits into from May 15, 2020

Conversation

roopakv
Copy link
Contributor

@roopakv roopakv commented Apr 29, 2020

This is a proposed changed for #646

@gbbr gbbr changed the title [Proposal][http] Add host contrib/internal/httputil: add HTTP host tag Apr 30, 2020
gbbr
gbbr reviewed Apr 30, 2020
View reviewed changes
Copy link
Contributor

@gbbr gbbr left a comment

Choose a reason for hiding this comment

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

LGTM, one small comment

ddtrace/ext/tags.go Outdated Show resolved Hide resolved
@gbbr gbbr added this to the 1.25.0 milestone Apr 30, 2020
@gbbr
Copy link
Contributor

gbbr commented Apr 30, 2020 •
edited

Please add a this tag to at least one test too.

@roopakv
Copy link
Contributor Author

roopakv commented Apr 30, 2020

what did you mean by a "this tag"

@gbbr
Copy link
Contributor

gbbr commented Apr 30, 2020

Have at least one of the tests assert that the "http.host" tag is set correctly.

@roopakv
Copy link
Contributor Author

roopakv commented Apr 30, 2020

@gbbr ahh thankyou, could you also add a label to this PR so that tests run on circle

@gbbr
Copy link
Contributor

gbbr commented Apr 30, 2020

They will run once you add your next commit.

@roopakv
Copy link
Contributor Author

roopakv commented Apr 30, 2020

@gbbr any chance you could pull this in for 1.24?

@roopakv
Copy link
Contributor Author

roopakv commented May 1, 2020

@gbbr done.

friendly ping on pulling in for v1.24 ?

gbbr
gbbr reviewed May 1, 2020
View reviewed changes
Copy link
Contributor

@gbbr gbbr left a comment

Choose a reason for hiding this comment

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

Thanks for your patience. It's too late for 1.24.0 since we've already started the release process for it a few days ago. I hope that's ok for you. If you need this change you can simply point your dependency manager to the commit - would that work?

contrib/internal/httputil/trace.go Outdated Show resolved Hide resolved
@roopakv
Copy link
Contributor Author

roopakv commented May 1, 2020

@gbbr updated. LMK if this looks ok

gbbr
gbbr reviewed May 4, 2020
View reviewed changes
Copy link
Contributor

@gbbr gbbr 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! This is great! Please bear with me through the review...

contrib/internal/httputil/trace_test.go Outdated Show resolved Hide resolved
gbbr
gbbr reviewed May 5, 2020
View reviewed changes
Copy link
Contributor

@gbbr gbbr left a comment

Choose a reason for hiding this comment

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

Wonderful! This is perfect! Just a few more nits.

contrib/internal/httputil/trace_test.go Outdated Show resolved Hide resolved
contrib/internal/httputil/trace_test.go Outdated Show resolved Hide resolved
contrib/internal/httputil/trace_test.go Outdated Show resolved Hide resolved
contrib/internal/httputil/trace_test.go Outdated Show resolved Hide resolved
roopakv
roopakv commented May 5, 2020
View reviewed changes
Copy link
Contributor Author

@roopakv roopakv left a comment

Choose a reason for hiding this comment

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

made all changes. LMK what you think

gbbr
gbbr previously approved these changes May 5, 2020
View reviewed changes
Copy link
Contributor

@gbbr gbbr 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 Roopak! I'm really happy with how this worked out. We will merge it after we "unfreeze" the master and release v1.24.0

knusbaum
knusbaum approved these changes May 15, 2020
View reviewed changes
Copy link
Contributor

@knusbaum knusbaum 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.
Thanks, @roopakv

@knusbaum knusbaum merged commit bb1893e into DataDog:v1 May 15, 2020
mingrammer pushed a commit to mingrammer/dd-trace-go that referenced this pull request Dec 22, 2020
@roopakv @mingrammer
contrib/internal/httputil: add HTTP host tag (DataDog#648)
96c0ebf
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@knusbaum knusbaum knusbaum approved these changes

@gbbr gbbr gbbr approved these changes

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

Successfully merging this pull request may close these issues.

None yet
3 participants
@roopakv @gbbr @knusbaum