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

Alternate implementation for issue #92 #118

Closed
wants to merge 1 commit into from

Conversation

mariusvniekerk
Copy link

@mariusvniekerk mariusvniekerk commented Jun 27, 2023

Fixes #92

This implementation more closely mirrors the default otelgrpc implementation in opentelemnetry-contrib

By default that implementation does not send events unless they are requested as part of the config.

Much of this work is merely copying and adjusting some of the upstream implementation.

This implementation more closely mirrors the default otelgrpc implementation in opentelemnetry-contrib
@CLAassistant
Copy link

CLAassistant commented Jun 27, 2023

CLA assistant check
All committers have signed the CLA.

Copy link

@DavidS-ovm DavidS-ovm left a comment

Choose a reason for hiding this comment

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

Yes, please!

@joshcarp
Copy link
Contributor

joshcarp commented Jul 7, 2023

Hey, so based on the pattern that we've already set in this codebase i'm more in favor of https://github.com/bufbuild/connect-opentelemetry-go/pull/98

We've encountered times where the opentelemetry specification doesn't suite our needs and we've added options to modify the behaviour; such as the introduction of WithoutServerPeerAttributes in https://github.com/bufbuild/connect-opentelemetry-go/pull/50

If we want the default behaviour to change then this should be opened up as an issue or pr in the opentelemetry specification.

@joshcarp joshcarp closed this Jul 7, 2023
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.

Excessive span.AddEvent for request/response size
4 participants