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

Allow retrieving span created in agent from otel, and store agent span in context when creating it with otel #1450

Merged
merged 3 commits into from
Jun 2, 2023
Merged

Allow retrieving span created in agent from otel, and store agent span in context when creating it with otel #1450

merged 3 commits into from
Jun 2, 2023

Conversation

dmathieu
Copy link
Member

@dmathieu dmathieu commented May 31, 2023 •
edited
Loading

Closes #1449

We didn't store the agent span/transaction into the context, as that doesn't happen automatically and requires a manual operation when using the agent directly.
When using otel, which does add it to every context, it makes more sense not to have to require folks to do that themselves though.

This also sets up a context wrapper in the agent, so spans created from the agent are stored in the context to be read by otel.

@dmathieu dmathieu requested review from a team May 31, 2023 07:53
@dmathieu
Copy link
Member Author

dmathieu commented Jun 2, 2023

I have also added a custom agent wrapper to this PR, so spans created from the agent can be retrieved within otel.
I didn't create a new PR, as the first commit in this one allows the TestLinkOtelToAgent test to pass.

@dmathieu dmathieu mentioned this pull request Jun 2, 2023
Closed
@dmathieu dmathieu changed the title Store the agent transaction and span into the context Allow retrieving span creating in agent from otel, and store agent span in context when creating it with otel Jun 2, 2023
axw
axw approved these changes Jun 2, 2023
View reviewed changes
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.

LGTM, just one query - I don't think it's a big deal though.

module/apmotel/tracer.go
@@ -86,6 +86,7 @@ func (t *tracer) Start(ctx context.Context, spanName string, opts ...trace.SpanS
Parent: tc,
Start: startTime,
})
ctx = apm.ContextWithSpan(ctx, s.span)
Copy link
Member

Choose a reason for hiding this comment

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

Rather than adding two span objects to context, should we be setting apm.OverrideSpanFromContext and apm.TransactionFromContext? The functions we set would look for a trace.Span in the context, type-asserting to *apmotel.span and extracting the transaction or span within.

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem with that is that if, someone is using both this module and the apmot one (it's unlikely I know), one will override the other.
I did it this way to prevent the module frome having to be exclusive with other ones using this wrapper.

Copy link
Member

Choose a reason for hiding this comment

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

OK. TBH I don't really care about that since OpenTracing is extremely deprecated, but your approach is fine. Happy to leave it as it is. Perhaps we should drop apmot in the next major version.

Copy link
Member Author

Choose a reason for hiding this comment

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

Another reason is that we have to care less about internals of the agent. We just let do its own stuff, and don't need to setup a context wrapper.

@dmathieu dmathieu changed the title Allow retrieving span creating in agent from otel, and store agent span in context when creating it with otel Allow retrieving span created in agent from otel, and store agent span in context when creating it with otel Jun 2, 2023
@dmathieu dmathieu merged commit b4c64f7 into elastic:main Jun 2, 2023
@dmathieu dmathieu deleted the apmotel-tx-span-context branch June 2, 2023 10:15
axw added a commit that referenced this pull request Jun 22, 2023
@axw @dmathieu
@simitt @SylvainJuge @dependabot
Merge v2.4.3 into 2.x (#1474)
3d4cc71 
* module/apmotel: update to stable metrics API (#1448)

* module/apmotel: update to stable metrics API

* module/apmotel: go mod download

* Allow retrieving span created in agent from otel, and store agent span in context when creating it with otel (#1450)

* store the agent transaction and span into the context

* add wrapper so we can retrieve agent span from otel

* fix precheck

* Require Go 1.19+ (#1453)

* Require Go 1.19

Update docs to drop the "best effort" support for
unsupported Go versions, and only test with the
two most recent Go releases. Also, simplify the
"check-modules" and "update-modules" make targets.

* Remove extraneous "go:build" tags

* README: update supported Go versions (#1463)

* Update https://github.com/stretchr/testify v1.8.4 (#1464)

* [doc] Align agents logs documentation (#1465)

* align logs docs

* fix typo

* add missing reference

* build(deps): bump github.com/gin-gonic/gin in /module/apmgin (#1451)

Bumps [github.com/gin-gonic/gin](https://github.com/gin-gonic/gin) from 1.7.7 to 1.9.1.
- [Release notes](https://github.com/gin-gonic/gin/releases)
- [Changelog](https://github.com/gin-gonic/gin/blob/master/CHANGELOG.md)
- [Commits](gin-gonic/gin@v1.7.7...v1.9.1)

---
updated-dependencies:
- dependency-name: github.com/gin-gonic/gin
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Update golang.org/x/... to latest (#1466)

* Update golang.org/x/sys to latest

* Update golang.org/x/*

* Update go-fastjson

* build(deps): bump mellium.im/sasl in /module/apmgopgv10 (#1467)

Bumps mellium.im/sasl from 0.2.1 to 0.3.1.

---
updated-dependencies:
- dependency-name: mellium.im/sasl
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* build(deps): bump github.com/prometheus/client_golang (#1468)

Bumps [github.com/prometheus/client_golang](https://github.com/prometheus/client_golang) from 1.7.0 to 1.11.1.
- [Release notes](https://github.com/prometheus/client_golang/releases)
- [Changelog](https://github.com/prometheus/client_golang/blob/main/CHANGELOG.md)
- [Commits](prometheus/client_golang@v1.7.0...v1.11.1)

---
updated-dependencies:
- dependency-name: github.com/prometheus/client_golang
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* build(deps): bump mellium.im/sasl from 0.2.1 to 0.3.1 in /module/apmgopg (#1373)

Bumps mellium.im/sasl from 0.2.1 to 0.3.1.

---
updated-dependencies:
- dependency-name: mellium.im/sasl
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* ci: use latest Go release for microbenchmarks (#1469)

* Fix data race accessing Request.URL (#1472)

If an HTTP client is instrumented with apmhttp,
then the http.Request.URL is recorded for including
in span context. The caller of the HTTP client request
may mutate the URL after the response is ready, so we
must clone the URL while recording it to prevent data
races.

* Prepare v2.4.3 (#1473)

---------

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: Damien Mathieu <[email protected]>
Co-authored-by: Silvia Mitter <[email protected]>
Co-authored-by: SylvainJuge <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@axw axw axw approved these changes

Assignees
No one assigned
Labels
agent-go
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Can't mix native transaction with Otel's trace.SpanFromContext
2 participants
@dmathieu @axw