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

Observability: Trace context propagation within DPF #1162

Merged

Conversation

marcgs
Copy link
Contributor

@marcgs marcgs commented Apr 14, 2022

What this PR changes/adds

Add trace context propagation at different points within DPF:

  • DataPlaneManagerImpl: trace context is propagated to the corresponding async worker threads through the "DataFlowRequest" entities stored in the processing queue.
  • ParallelSink: trace context is propagated to the async workers that process parts. No entity is persisted in this case, the trace context is propagated in memory.

Additionally TracingIntegrationTest was extended to verify traces propagation within DPF is working properly.

Why it does that

In order to have traces spanning across asynchronous calls it is required to propagate the tracing context to worker threads. See resulting trace visualization in Jaeger:

trace

Linked Issue(s)

Closes #936

Checklist

  • added appropriate tests?
  • performed checkstyle check locally?
  • added/updated copyright headers?
  • documented public classes/methods?
  • added/updated relevant documentation?
  • added relevant details to the changelog? (skip with label no-changelog)
  • formatted title correctly? (take a look at the CONTRIBUTING and styleguide for details)

@marcgs marcgs changed the title Feature/936 observability dpf Observability: Trace context propagation within DPF Apr 14, 2022
@marcgs marcgs force-pushed the feature/936-observability-dpf branch from 3b0eaa3 to 4d64fc7 Compare April 16, 2022 10:29
@marcgs marcgs marked this pull request as ready for review April 16, 2022 11:35
@marcgs marcgs requested a review from jimmarino as a code owner April 16, 2022 11:35
@bscholtes1A
Copy link
Contributor

Hi @marcgs
I am a bit embarassed with the fact of adding a dependency on opentelemetry into the *-spi. For example data-plane-transfer has a dependency on data-plane-spi. With your PR, this would therefore pull the opentelemetry dependency into data-plane-transfer.

Is there any way to limit dependencies on external libraries to non-spi submodules?

@marcgs
Copy link
Contributor Author

marcgs commented Apr 21, 2022

Hi @marcgs I am a bit embarassed with the fact of adding a dependency on opentelemetry into the *-spi. For example data-plane-transfer has a dependency on data-plane-spi. With your PR, this would therefore pull the opentelemetry dependency into data-plane-transfer.

Is there any way to limit dependencies on external libraries to non-spi submodules?

HI @bscholtes1A , thanks for your comment :-). All open-telemetry dependencies have implementation scope, which actually prevents from the situation you have described. This PR is actually not adding any dependencies in the spi module either, these changes were already discussed and agreed with @jimmarino and @paullatzelsperger in previous PRs and documented in a decision record.

@marcgs marcgs force-pushed the feature/936-observability-dpf branch from 4d64fc7 to bcb3863 Compare April 26, 2022 07:20
@marcgs marcgs force-pushed the feature/936-observability-dpf branch from bcb3863 to 1f5782c Compare April 26, 2022 07:35
@marcgs
Copy link
Contributor Author

marcgs commented Apr 26, 2022

@bscholtes1A @jimmarino Could we merge this PR as it already has 2 approvals?

@bscholtes1A bscholtes1A merged commit 9282a7d into eclipse-edc:main Apr 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Observability: Observability for DPF
3 participants