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

proposal for correlating synthetics traces #825

Merged
merged 7 commits into from
Sep 27, 2023

Conversation

vigneshshanmugam
Copy link
Member

specs/integrations/synthetics.md Outdated Show resolved Hide resolved
specs/integrations/synthetics.md Outdated Show resolved Hide resolved
specs/integrations/synthetics.md Outdated Show resolved Hide resolved
specs/integrations/synthetics.md Outdated Show resolved Hide resolved
specs/integrations/synthetics.md Outdated Show resolved Hide resolved
specs/integrations/synthetics.md Outdated Show resolved Hide resolved
specs/integrations/synthetics.md Outdated Show resolved Hide resolved
specs/integrations/synthetics.md Outdated Show resolved Hide resolved
specs/integrations/synthetics.md Outdated Show resolved Hide resolved
specs/integrations/synthetics.md Outdated Show resolved Hide resolved
specs/integrations/synthetics.md Outdated Show resolved Hide resolved
specs/integrations/synthetics.md Outdated Show resolved Hide resolved
- `http.headers.user-agent`:
- Contains `Elastic/Synthetics` for all outgoing requests from Synthetis based monitors.

There is a limitation with this approach
Copy link
Member

Choose a reason for hiding this comment

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

Another challenge is that native OTel agents don't capture all HTTP headers by default, IINM.

Copy link
Member Author

@vigneshshanmugam vigneshshanmugam Sep 7, 2023

Choose a reason for hiding this comment

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

I believe we cant do much here except the users would see a rootless trace unless Synthetics itself is also instrumented using APM?

I can add this to the limitation list if its a huge concern?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think it's a huge concern but we should still add it to the limitations.
The concern is not necessarily about the trace being rootless. I think it's more about that we aren't able to tell synthetics requests apart from normal requests. I think that's a limitation we can live with but it doesn't allow us to build features like "show only requests from synthetics".

Copy link
Contributor

@gregkalapos gregkalapos Sep 27, 2023

Choose a reason for hiding this comment

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

The requirement level of the user_agent.original attribute (which is the Value of the HTTP User-Agent header sent by the client.) is recommended. I also think this is trivial to capture, so I expect most OTel implementations will have this out of the box.

Furthermore, what we could also do is this: for some languages, it's likely we'll end up with custom OTel distributions - with that we have control on how the OTel agent is configured. To be on a safe-side, we could add something like this to this spec:

Custom Elastic OTel agent distributions MUST capture the user_agent.original attribute in order to enable APM-Synthetic correlation.

In most cases, this will have no effect, since I expect vanilla OTel agents already doing so. Nevertheless with this we can mitigate the issue of not being able to recognize synthetic calls in OTel Agents.

specs/integrations/synthetics.md Outdated Show resolved Hide resolved
vigneshshanmugam and others added 2 commits September 7, 2023 06:37
Co-authored-by: Felix Barnsteiner <felixbarny@users.noreply.github.com>
Copy link
Member

@felixbarny felixbarny left a comment

Choose a reason for hiding this comment

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

A couple of smaller nits and suggestions. Overall LGTM.

specs/integrations/synthetics.md Outdated Show resolved Hide resolved
specs/integrations/synthetics.md Outdated Show resolved Hide resolved
specs/integrations/synthetics.md Outdated Show resolved Hide resolved
- `http.headers.user-agent`:
- Contains `Elastic/Synthetics` for all outgoing requests from Synthetis based monitors.

There is a limitation with this approach
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it's a huge concern but we should still add it to the limitations.
The concern is not necessarily about the trace being rootless. I think it's more about that we aren't able to tell synthetics requests apart from normal requests. I think that's a limitation we can live with but it doesn't allow us to build features like "show only requests from synthetics".

specs/integrations/synthetics.md Outdated Show resolved Hide resolved
specs/integrations/synthetics.md Outdated Show resolved Hide resolved
Copy link
Contributor

@gregkalapos gregkalapos left a comment

Choose a reason for hiding this comment

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

Looks very nice 👍

As already discussed on slack, I agree with the traceparent approach instead of using baggage in order to mitigate the challenges caused by potentially unsampled traces.

Left some comments.

specs/integrations/synthetics.md Outdated Show resolved Hide resolved
specs/integrations/synthetics.md Outdated Show resolved Hide resolved
vigneshshanmugam and others added 2 commits September 27, 2023 09:53
Co-authored-by: Felix Barnsteiner <felixbarny@users.noreply.github.com>
@vigneshshanmugam vigneshshanmugam merged commit a3510c0 into main Sep 27, 2023
1 check passed
@vigneshshanmugam vigneshshanmugam deleted the synthetics-correlation branch September 27, 2023 19:21
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.

4 participants