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

Buffer limit API for message events, links, and annotations? #138

Closed
jgracenin opened this issue Jul 13, 2018 · 6 comments
Closed

Buffer limit API for message events, links, and annotations? #138

jgracenin opened this issue Jul 13, 2018 · 6 comments

Comments

@jgracenin
Copy link

I have a Go service with a long running stream that had a memory leak due to unbounded storage of
message events. This was a known limitation: census-instrumentation/opencensus-go#401

gRPC's MAX_CONNECTION_AGE would help, but I also made a quick fork with a limit on message events, annotations, and links: census-instrumentation/opencensus-go#824 .

Do any other language implementations already limit these internal structures yet (events, links annotations) yet?

The PR also sparked a larger discussion on more granular limits for some future features (e.g. classified events, links annotations), but that's a larger API change and is probably better to be discussed here.

@rakyll
Copy link
Contributor

rakyll commented Jul 13, 2018

/cc @sebright @g-easy @songy23

@songy23
Copy link
Contributor

songy23 commented Jul 13, 2018

Do any other language implementations already limit these internal structures yet (events, links annotations) yet?

Yes, in Java we have some default limit for them:

  private static final int DEFAULT_SPAN_MAX_NUM_ATTRIBUTES = 32;
  private static final int DEFAULT_SPAN_MAX_NUM_ANNOTATIONS = 32;
  private static final int DEFAULT_SPAN_MAX_NUM_MESSAGE_EVENTS = 128;
  private static final int DEFAULT_SPAN_MAX_NUM_LINKS = 128;

Users can override these numbers with TraceConfig.updateActiveTraceParams.

@rakyll
Copy link
Contributor

rakyll commented Jul 13, 2018

@songy23, can we update https://github.com/census-instrumentation/opencensus-specs/blob/master/trace/TraceConfig.md to represent this? So, other languages can refer to the spec when implementing.

@songy23
Copy link
Contributor

songy23 commented Jul 13, 2018

Sure, working on a PR now.

@g-easy
Copy link
Contributor

g-easy commented Jul 16, 2018

#138 (comment) FYI C++ has the same.

@bogdandrutu
Copy link
Contributor

I think the specs has limits now. This needs to be implemented in Go and other languages now.

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

No branches or pull requests

5 participants