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

feat: make spans fit an envelope size #4022

Merged
merged 4 commits into from
Sep 17, 2024
Merged

Conversation

mathnogueira
Copy link
Member

@mathnogueira mathnogueira commented Sep 17, 2024

This PR makes all polling responses to be added to an envelope and to fit a certain size. It tries to fit as many spans as possible in the envelope before sending it to the server. In case a span is let out of the envelope, it will be sent in the next polling iterations.

Fixes

  • grpc was breaking due to max message size

Checklist

  • tested locally
  • added new dependencies
  • updated the docs
  • added a test

Loom video

I could not record a loom because it is not supported in firefox 😢 But here is a screencast. In this case, I was running the agent in debug mode and an envelope max size of 5kb, this is why you see spans increasing slowly. By default, we are going to use 3MB, which would be more than enough for real-life spans.

Screencast.from.2024-09-17.16-56-18.webm

Copy link

vercel bot commented Sep 17, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
tracetest ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 17, 2024 8:45pm

@@ -24,6 +25,8 @@ import (
"go.uber.org/zap"
)

const maxEnvelopeSize = 4 * 1024 * 1024 // 4MB
Copy link
Member Author

@mathnogueira mathnogueira Sep 17, 2024

Choose a reason for hiding this comment

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

I wonder if we should increase this value. What do you think @xoscar, @schoren and @danielbdias? Like 10MB. It should be something less than the server limit due to the large span policy in the code.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You mean increasing the value server side too?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, just here. The server is already accepting a large-enough message. Increasing the message here means that in we can send more spans per polling cycle.

Copy link
Member Author

Choose a reason for hiding this comment

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

We discussed and the limit on the server is 4MB, so I'm setting this to be 3MB to prevent an overflow in case of a large span as discussed in the code.

Copy link
Collaborator

@xoscar xoscar left a comment

Choose a reason for hiding this comment

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

Great job!


// There's a weird scenario that must be covered here: imagine a span so big it is bigger than maxPacketSize.
// It is impossible to send a span like this, so in this case, we classify those spans as "large spans" and we allow
// `largeSpansPerPacket` per packet.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a great point, I believe there is an issue around this very same subject in one of the otel collector repos, I'm glad you thought about it too!

@@ -24,6 +25,8 @@ import (
"go.uber.org/zap"
)

const maxEnvelopeSize = 4 * 1024 * 1024 // 4MB
Copy link
Collaborator

Choose a reason for hiding this comment

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

You mean increasing the value server side too?

@mathnogueira mathnogueira merged commit 1f45068 into main Sep 17, 2024
40 checks passed
@mathnogueira mathnogueira deleted the feat/agent-polling-envelope branch September 17, 2024 20:59
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.

2 participants