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

Handle invalid argument, unavailable ErrorCodes (and refactor OTLP+Arrow receiver tests) #15

Merged
merged 8 commits into from
Nov 18, 2022

Conversation

jmacd
Copy link
Contributor

@jmacd jmacd commented Nov 16, 2022

Description:

Changes the Receiver to return StatusMessage responses to the sender for non-gRPC errors. These include:

  1. UNAVAILABLE: the Consume(Logs|Metrics|Traces) function returned an error
  2. INVALID_ARGUMENT: the Arrow adapter returned an error translating the data

The adapter library added metrics support in recent PRs, this uncomments the associated receiver handling in this PR. (Exporter handling will be done separately.)

Testing: Improves test structure and coverage for OTLP+Arrow receiver by using a mock Arrow adapter and channels to better distinguish normal shutdown (via context.Cancel) from abnormal shutdown (via gRPC send/recv errors).

Depends on f5/otel-arrow-adapter#52

Part of #21.

@jmacd jmacd requested review from lquerel and jaronoff97 November 16, 2022 23:39
@jmacd jmacd changed the title Refactor OTLP+Arrow receiver tests Handle invalid argument, unavailable ErrorCodes (and refactor OTLP+Arrow receiver tests) Nov 16, 2022
@codecov
Copy link

codecov bot commented Nov 17, 2022

Codecov Report

Base: 90.64% // Head: 90.83% // Increases project coverage by +0.18% 🎉

Coverage data is based on head (d139e07) compared to base (5121f43).
Patch coverage: 70.37% of modified lines in pull request are covered.

❗ Current head d139e07 differs from pull request most recent head 495178a. Consider uploading reports for the commit 495178a to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #15      +/-   ##
==========================================
+ Coverage   90.64%   90.83%   +0.18%     
==========================================
  Files         241      241              
  Lines       13975    13978       +3     
==========================================
+ Hits        12668    12697      +29     
+ Misses       1065     1042      -23     
+ Partials      242      239       -3     
Impacted Files Coverage Δ
receiver/otlpreceiver/otlp.go 75.14% <0.00%> (ø)
receiver/otlpreceiver/internal/arrow/arrow.go 80.45% <73.07%> (+25.69%) ⬆️
receiver/otlpreceiver/internal/arrow/mock/mock.go 77.27% <0.00%> (+7.57%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link

@lquerel lquerel left a comment

Choose a reason for hiding this comment

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

Few suggestions.
LGTM

receiver/otlpreceiver/internal/arrow/arrow.go Outdated Show resolved Hide resolved
receiver/otlpreceiver/internal/arrow/arrow.go Outdated Show resolved Hide resolved
Copy link
Contributor

@jaronoff97 jaronoff97 left a comment

Choose a reason for hiding this comment

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

I have two non-blocking questions, otherwise this looks great :)

receiver/otlpreceiver/internal/arrow/arrow.go Show resolved Hide resolved
receiver/otlpreceiver/internal/arrow/arrow.go Outdated Show resolved Hide resolved
@jmacd jmacd merged commit 02d27dd into open-telemetry:main Nov 18, 2022
@jmacd jmacd deleted the jmacd/rtest2 branch November 18, 2022 17:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants