Skip to content
This repository has been archived by the owner on Oct 3, 2023. It is now read-only.

TraceParams are not mentioning the buffer limit #147

Open
rakyll opened this issue Jul 18, 2018 · 4 comments
Open

TraceParams are not mentioning the buffer limit #147

rakyll opened this issue Jul 18, 2018 · 4 comments
Labels

Comments

@rakyll
Copy link
Contributor

rakyll commented Jul 18, 2018

There is a requirement to set the default buffer size globally (see census-instrumentation/opencensus-go#824) but this setting is not represented in TraceParams.

In order to have a more predictable API from user's perspective, would it make sense to represent this setting in the TraceConfig?

/cc @Ramonza @bogdandrutu @songy23

@songy23
Copy link
Contributor

songy23 commented Jul 18, 2018

In Java we have a fixed buffer size of 32, though this looks like an implementation detail. Buffer size is not configurable and is not part of TraceParams in Java.

@songy23 songy23 added the trace label Jul 18, 2018
@semistrict
Copy link
Contributor

I don't know if adding a default limit for all types makes sense. Why not just stick with individual limits for events, links, attributes, etc.?

@jgracenin
Copy link

Hey -- I will try my best to clear up the confusion here.

@songy23 linked to a static trace export buffer size, which is not what the Go PR is about.

As of right now, here is the limit support for attributes, annotations, message events, links:

Global limits that can be overridden:

Per-span limits that can be overridden:

The spec was recently updated in #139 and then again in #141.

I find the wording in the spec still a bit ambiguous -- if we need global limit overrides AND per-span limit overrides, or just the latter.

My updated CL for Go for the new spec (intentionally not pushed yet) implements BOTH global limit overrides and per-span limit overrides. Per-span limits override any global limit regardless if it was the default value or changed.

I asked for clarification this morning and held off on pushing the updated CL in hopes to not make the situation any more confusing than it already is :)

@bogdandrutu
Copy link
Contributor

@jgracenin thanks for the clarification.

My 2 cents here:

  1. I think we need a global limit - third-party libraries that are instrumented and used in the binary may not set the correct limits and you want to have the ability to control that without changing the library code.
  2. Having a way to configure for a specific span is great especially if you have a background thread that runs long and you want larger buffers just for this.
  3. I personally think we should have 3 "levels" for global limits: default, custom, forced_custom. Only the forced_custom overrides the per-span limits - I can see this as a "big red button" to stop the damage.

Anyone has an opinion about this?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

5 participants