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

Mark frontend server spans as synthetic when coming from loadgen #1181

Closed

Conversation

cedricziel
Copy link
Contributor

Changes

This change will change the way synthetic requests work:

  • frontend service will no longer restart the trace
  • frontend-proxy will be visible in all traces
  • loadgen will be visible in all synthetic traces

This will ensure that service-graphs can correctly be built and we dont artificially hide services, but instead a trace will contain all the hops.

Merge Requirements

For new features contributions please make sure you have completed the following
essential items:

  • CHANGELOG.md updated to document new feature additions
  • Appropriate documentation updates in the [docs][]

Related: #1180

This change will change the way synthetic requests work:
* frontend service will no longer restart the trace
* frontend-proxy will be visible in all traces
* loadgen will be visible in all synthetic traces

This will ensure that service-graphs can correctly be built and we dont artificially hide
services, but instead a trace will contain all the hops.
@cedricziel cedricziel requested a review from a team October 14, 2023 17:29
@julianocosta89
Copy link
Member

If we add this change we will drop the example for linked traces.

@cedricziel
Copy link
Contributor Author

I am sure we can come up with a better one. The current state is hiding too much behind the span link.

I tried looking if we could configure envoy to restart and link, but to no avail :-/

@puckpuck
Copy link
Contributor

We split the traces to showcase trace linking early in the demo's development. Though the traces look nicer when not linked, we specifically try to showcase the OpenTelemetry functionality this PR is removing.

@cedricziel
Copy link
Contributor Author

cedricziel commented Oct 16, 2023

@puckpuck I think we it's less about looks, I found it to be confusing when the service graph and trace didn't contain the frontend-proxy. I think we can probably highlight this capability in a better place and I'd be happy to add it somewhere else.

Can you think of a better place that doesn't hide a critical piece of the puzzle? Maybe in a downstream message consumer?

@julianocosta89
Copy link
Member

@puckpuck I think we it's less about looks, I found it to be confusing when the service graph and trace didn't contain the frontend-proxy. I think we can probably highlight this capability in a better place and I'd be happy to add it somewhere else.

Can you think of a better place that doesn't hide a critical piece of the puzzle? Maybe in a downstream message consumer?

Quoting the OTel docs here, it seems that the link would be a better fit for the message queue indeed:

Span Links

Links exist so that you can associate one span with one or more spans, implying a causal relationship. For example, let’s say we have a distributed system where some operations are tracked by a trace.

In response to some of these operations, an additional operation is queued to be executed, but its execution is asynchronous. We can track this subsequent operation with a trace as well.

We would like to associate the trace for the subsequent operations with the first trace, but we cannot predict when the subsequent operations will start. We need to associate these two traces, so we will use a span link.

You can link the last span from the first trace to the first span in the second trace. Now, they are causally associated with one another.

Links are optional but serve as a good way to associate trace spans with one another.

I'd be okay with that change.

@puckpuck
Copy link
Contributor

I'm a little hesitant to remove something (child spans for accounting and frauddetection) from our existing traces, but not completely against the idea.

I think it makes sense to have an issue to discuss a better spot for span links and hold off on this PR until we get that resolved and implemented.

@cartersocha
Copy link
Contributor

Vendor support for span link visualization is patchy although we should encourage more representation. Does jaeger display span links well or is just an attribute?

@puckpuck
Copy link
Contributor

Jaeger does support Span Links. Honeycomb does as well.

@cartersocha
Copy link
Contributor

Cool. I’m okay with the change

@julianocosta89
Copy link
Member

Dynatrace also supports it

@cedricziel
Copy link
Contributor Author

Conflicts resolved.

@austinlparker
Copy link
Member

Could you post a screenshot of what you're seeing without this PR? I'm a bit confused on if this is working as intended or not.

Copy link

github-actions bot commented Nov 7, 2023

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Nov 7, 2023
@github-actions github-actions bot removed the Stale label Nov 8, 2023
Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Nov 15, 2023
@github-actions github-actions bot removed the Stale label Nov 16, 2023
@julianocosta89
Copy link
Member

@cedricziel, did you see this comment?
#1181 (comment)

@cedricziel
Copy link
Contributor Author

Ahh. Dropped the ball here, sorry. Thanks @julianocosta89 !

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.

6 participants