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

Handle partial-success responses for OTLP trace #3106

Merged
merged 22 commits into from
Sep 6, 2022

Conversation

jmacd
Copy link
Contributor

@jmacd jmacd commented Aug 23, 2022

Fixes #3102.

This constructs an error for passing to otel.Handle() from any of the new PartialSuccess messages in OTLP v0.19.

See the specification of this feature.

@codecov
Copy link

codecov bot commented Aug 23, 2022

Codecov Report

Merging #3106 (96eadb0) into main (9c2a0c2) will increase coverage by 0.0%.
The diff coverage is 80.3%.

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #3106   +/-   ##
=====================================
  Coverage   76.4%   76.4%           
=====================================
  Files        179     180    +1     
  Lines      11977   12014   +37     
=====================================
+ Hits        9160    9190   +30     
- Misses      2578    2583    +5     
- Partials     239     241    +2     
Impacted Files Coverage Δ
exporters/otlp/otlptrace/otlptracehttp/client.go 76.2% <64.2%> (-0.8%) ⬇️
exporters/otlp/otlptrace/otlptracegrpc/client.go 92.5% <100.0%> (+0.3%) ⬆️
exporters/otlp/partialsuccess.go 100.0% <100.0%> (ø)
exporters/jaeger/jaeger.go 90.3% <0.0%> (-0.9%) ⬇️

exporters/otlp/otlpmetric/otlpmetricgrpc/client.go Outdated Show resolved Hide resolved
exporters/otlp/otlpmetric/otlpmetrichttp/client.go Outdated Show resolved Hide resolved
exporters/otlp/otlpmetric/otlpmetrichttp/client_test.go Outdated Show resolved Hide resolved
exporters/otlp/otlpmetric/otlpmetricgrpc/client_test.go Outdated Show resolved Hide resolved
jmacd and others added 3 commits August 24, 2022 12:57
Co-authored-by: David Ashpole <dashpole@google.com>
Co-authored-by: David Ashpole <dashpole@google.com>
@jmacd
Copy link
Contributor Author

jmacd commented Aug 30, 2022

Please consider 54d9dcf a question about readability. I rewrote the code to have better coverage (to pass the CI checks), but I'm not sure it's a good trade. OTOH, I feel it's not worth adding specialized tests to ensure that errors-in-response-handling (protobuf error, I/O error, etc.) are covered. What do you think?

@dashpole
Copy link
Contributor

I also prefer the lower-code-coverage version for readability. I would support merging even if it does not pass those checks. Was it failing the required check, or the non-required check?

Copy link
Contributor

@MrAlias MrAlias left a comment

Choose a reason for hiding this comment

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

Can we pull the changes to the otlpmetricgrpc package out of the PR? They will conflict with #3094

@jmacd
Copy link
Contributor Author

jmacd commented Aug 31, 2022

I'm not sure anyone is interested in seeing this supported for traces -- from my perspective we may as well wait until after #3094. Thanks @MrAlias.

This reverts commit 54d9dcf.
Mixed results from the style question. We prefer less test, more readability it seems.
@jmacd
Copy link
Contributor Author

jmacd commented Aug 31, 2022

I reverted the otlpmetric portion. I also reverted the questionable style in 54d9dcf. (This is no longer a priority for me, however 😁)

@jmacd jmacd changed the title Handle partial-success responses for OTLP trace and metrics Handle partial-success responses for OTLP trace ~and metrics~ Aug 31, 2022
@jmacd jmacd changed the title Handle partial-success responses for OTLP trace ~and metrics~ Handle partial-success responses for OTLP trace Aug 31, 2022
Copy link
Contributor

@MadVikingGod MadVikingGod left a comment

Choose a reason for hiding this comment

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

LGTM. The only thing that needs addressing is the closing of the http.body.

exporters/otlp/otlptrace/otlptracegrpc/client_test.go Outdated Show resolved Hide resolved
exporters/otlp/otlptrace/otlptracehttp/client.go Outdated Show resolved Hide resolved
exporters/otlp/otlptrace/otlptracehttp/client_test.go Outdated Show resolved Hide resolved
// See the License for the specific language governing permissions and
// limitations under the License.

package otlp // import "go.opentelemetry.io/otel/exporters/otlp"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm conflicted about where this should live. On the one hand, I would like users to create an ErrorHandler that does something more intelligent than just log a string. On the other hand, I would prefer not to expose a new API surface needlessly.

If no one has any other objections to this than I can live with it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm a fan of structured logging; I would advocate for a backwards-compatible mechanism to accomplish it. The PartialSuccess error can implement some future API that enables it to be expressed in a structured way when the producer logs it to a structure-aware logger.

I didn't know where to put it either. At this level in the current package structure, it falls into the top-level package, which feels OK to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

With no objections to this, we can leave it where it is.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would rather have these live in an internal package, at lease to start.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what a user will be able to do with an error handler other than log the error given they cannot retry the export. And adding this to the public API seems like adding something only OTel developers will use or care about, not users.

jmacd and others added 4 commits September 1, 2022 09:13
Co-authored-by: Aaron Clawson <3766680+MadVikingGod@users.noreply.github.com>
Co-authored-by: Aaron Clawson <3766680+MadVikingGod@users.noreply.github.com>
@MadVikingGod MadVikingGod merged commit 569f743 into open-telemetry:main Sep 6, 2022
@MrAlias
Copy link
Contributor

MrAlias commented Sep 6, 2022

There was an unresolved comment: #3106 (comment)

I would rather have these live in an internal package, at lease to start.

I think this PR was merged pre-maturely.

@MrAlias
Copy link
Contributor

MrAlias commented Sep 6, 2022

There was an unresolved comment: #3106 (comment)

I would rather have these live in an internal package, at lease to start.

I think this PR was merged pre-maturely.

I'm opening a to address this.

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.

Handle partial success responses from OTLP export services
6 participants