-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
[otelarrowreceiver, otelarrowexporter] Add internal/grpcutil package #33688
Conversation
…eout in the syntax of gRPC
Same as open-telemetry/opentelemetry-collector-contrib#33688 but merging this code here will let me create and test a PR in that repository, whereas it will be messy to build off this work in the same repository. I expect this package to be deleted after open-telemetry/opentelemetry-collector-contrib#33688 and open-telemetry/opentelemetry-collector-contrib#33579 merged, as discussed in #225. Part of #227.
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
…tor-contrib into jmacd/arrowdeadline
This is part of open-telemetry/otel-arrow#227. We were encouraged to move development of the otel-arrow components into this repository, but we need our PRs to merge for this to continue. :-) |
…tor-contrib into jmacd/arrowdeadline
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. It'd be nice if a maintainer could double check the contents of NOTICE.
@mwear I would be willing to move this module under |
For the record, the ASLv2 section 4(b) directs us to do this. There was already a NOTICE file concerning bits of gopsutil that were copied, and I just copied the pattern.
|
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 - my take is this is an internal package, it helps now, ok to get it in
…tor-contrib into jmacd/arrowdeadline
…collector-contrib into jmacd/arrowdeadline
🥺 I think this could merge? |
Please merge this. |
I apologize for pushing this PR without apparently much cause. As we know, it is difficult to maintain multi-module PRs in this repository especially when introducing a new module. I could send a PR containing this PR combined with changes in otelarrowreceiver and one in otelarrowexporter. I could, but it would become harder to maintain and harder to review, so I've been waiting for this one to merge before sending more PRs. open-telemetry/otel-arrow#227 |
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.
Looks good to me overall, I don't have concerns with adding additional internal modules.
I'm not totally clear on the license requirements, but what's here seems sufficient to me. I'll merge this after the release is completed tomorrow if there aren't further comments.
…collector-contrib into jmacd/arrowdeadline
@evan-bradley is this good to merge? |
Description:
With utilities to encode and decode timeout in the syntax specified by gRPC.
This will be used by the otelarrow components to encode and decode timeout values.
In that sense, the new code could also be added into the pending internal/otelarrow package, see #33579
As this code is derived from gRPC-Go, some text is added in
NOTICE
according to the license. The code that this was derived from contained a TODO, which was largely addressed by eliminating very short timeouts from being encoded. This code will encode timeouts in milliseconds, seconds, hours, and days but it will not encode microsecond/nanosecond durations, these are rounded to0m
. The decoder will handle all durations that gRPC specifies, so that this logic can change in the future.Link to tracking Issue: open-telemetry/otel-arrow#227
Testing: Adds basic tests.