-
Notifications
You must be signed in to change notification settings - Fork 19
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
Exporter README, receiver README updates #29
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - found some typos and just had some basic questions for my understanding
- `recevier_recv`: uncompressed bytes received, prior to compression | ||
- `recevier_recv_wire`: compressed bytes received, on the wire. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/recevier_recv/receiver_recv
s/recevier_recv_wire/receiver_recv_wire
- `disabled`: disables use of Arrow, causing the exporter to use standard OTLP | ||
- `disable_downgrade`: prevents this exporter from using standard OTLP |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we mention what the default value is for these settings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@moh-osman3 FYI I observed one local test flake on |
Here at Lightstep we think that round_robin is a reasonably good default load balancer for OTel Arrow, while `pick_first` is what we get without setting something in this field. `pick_first` is not a reasonably good default load balancer for OTel Arrow. This change was documented in #29 already. Fixes #32.
After merging #24, unit tests for otelarrowreceiver were failing and I did not catch that. This PR fixes the unit tests by adding in support for an extra call to Send() the receiver will do (with arrowpb.StatusCode_STREAM_SHUTDOWN), to signal shutdown to the client. Addresses #29 (comment)
Release v0.3.0 includes: - #24 - #29 - #30 - #36 This release includes a BREAKING CHANGE 🛑 🛑 🛑 🛑 🛑. We do not anticipate another of these, it merely renames/enumbers status codes to match the gRPC status code numbering scheme. However, since status code 0 stays the same, it will result in unusual looking errors but should be safe to deploy the receivers first. Their CANCELED (i.e., SERVER_SHUTDOWN) will read as UNAVAILABLE, etc. See #27.
This flushes out the exporter README and makes a few corresponding fixes to the receiver README.