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

[apmhttp] WithClientSpanType ClientOption #1106

Merged
merged 10 commits into from
Aug 26, 2021

Conversation

Jasonfran
Copy link
Contributor

Hi,

This is a pull request to add a WithClientSpanType option when wrapping an http.Client

I've come across a few instances in the past where I want to better differentiate my HTTP requests. For example when calling into a Varnish cache over HTTP, I'd rather it have the "cache" span type. There are some cases where I have HTTP requests that are better classified as database requests, so I'd like the "db" span type.

Currently the workaround is to grab the span off the context and set the span type before closing the response body. It works, but it's not particularly elegant. That's why I think this new ClientOption would be useful.

I did the change before making an issue asking about it since it's a tiny change

Let me know 😄


By the way, TestFilesystemContextSetter and TestFilesystemContextSetterFileNotFound don't seem to work on my machine. It's a Windows machine. They result in these test failures:

=== RUN   TestFilesystemContextSetter
    context_test.go:91: ContextLine differs:   string(
        - 	`	panic("context!")`,
        + 	"\tpanic(\"context!\")\r",
          )
--- FAIL: TestFilesystemContextSetter (0.00s)
=== RUN   TestFilesystemContextSetterFileNotFound
    context_test.go:91: ContextLine differs:   string(
        - 	`	panic("context!")`,
        + 	"\tpanic(\"context!\")\r",
          )
--- FAIL: TestFilesystemContextSetterFileNotFound (0.00s)

@apmmachine
Copy link
Contributor

apmmachine commented Aug 19, 2021

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2021-08-26T00:43:01.527+0000

  • Duration: 32 min 21 sec

  • Commit: 4545713

Test stats 🧪

Test Results
Failed 0
Passed 11023
Skipped 268
Total 11291

Trends 🧪

Image of Build Times

Image of Tests

Copy link
Member

@axw axw 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, thanks @Jasonfran! Would you please add a short entry in CHANGELOG.asciidoc?

Not sure what's up with the tests on Windows, but that's essentially defunct code that we're keeping around to avoid breaking the API. I wouldn't worry about it.

module/apmhttp/client.go Show resolved Hide resolved
@Jasonfran
Copy link
Contributor Author

Thanks @axw I've updated the CHANGELOG.asciidoc, I think I've done it correctly. Does the docs/upgrading.asciidoc file need updating too?

Copy link
Member

@axw axw left a comment

Choose a reason for hiding this comment

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

Just a minor issue with the changelog update. Thanks and sorry, I should have given some guidance on how to update it! We will move things from "unreleased" to a new version when a new version is released.

CHANGELOG.asciidoc Outdated Show resolved Hide resolved
CHANGELOG.asciidoc Outdated Show resolved Hide resolved
@Jasonfran
Copy link
Contributor Author

Hi, I think that's done now?

@axw
Copy link
Member

axw commented Aug 26, 2021

LGTM, thanks for the PR :)

@axw axw enabled auto-merge (squash) August 26, 2021 00:43
@axw axw disabled auto-merge August 26, 2021 01:55
@axw axw merged commit 95ef283 into elastic:master Aug 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants