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

[otelarrowreceiver, otelarrowexporter] Add internal/grpcutil package #33688

Merged
merged 15 commits into from
Aug 6, 2024

Conversation

jmacd
Copy link
Contributor

@jmacd jmacd commented Jun 20, 2024

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 to 0m. 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.

@jmacd jmacd marked this pull request as ready for review June 21, 2024 16:28
@jmacd jmacd requested review from a team and fatsheep9146 June 21, 2024 16:28
jmacd added a commit to open-telemetry/otel-arrow that referenced this pull request Jun 21, 2024
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.
Copy link
Contributor

github-actions bot commented Jul 6, 2024

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Jul 6, 2024
@jmacd
Copy link
Contributor Author

jmacd commented Jul 8, 2024

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. :-)

@github-actions github-actions bot removed the Stale label Jul 9, 2024
Copy link
Member

@mwear mwear left a 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.

@jmacd
Copy link
Contributor Author

jmacd commented Jul 19, 2024

@mwear I would be willing to move this module under internal/otelarrow as well. It's the same owners. It's meant for the two OTel-Arrow components to use, but it could be useful outside OTel-Arrow.

@jmacd
Copy link
Contributor Author

jmacd commented Jul 19, 2024

It'd be nice if a maintainer could double check the contents of NOTICE.

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.

      (d) If the Work includes a "NOTICE" text file as part of its
          distribution, then any Derivative Works that You distribute must
          include a readable copy of the attribution notices contained
          within such NOTICE file, excluding those notices that do not
          pertain to any part of the Derivative Works, in at least one
          of the following places: within a NOTICE text file distributed
          as part of the Derivative Works; within the Source form or
          documentation, if provided along with the Derivative Works; or,
          within a display generated by the Derivative Works, if and
          wherever such third-party notices normally appear. The contents
          of the NOTICE file are for informational purposes only and
          do not modify the License. You may add Your own attribution
          notices within Derivative Works that You distribute, alongside
          or as an addendum to the NOTICE text from the Work, provided
          that such additional attribution notices cannot be construed
          as modifying the License.

Copy link
Contributor

@atoulme atoulme left a 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

@jmacd jmacd changed the title Add internal/grpcutil package [otelarrowreceiver, otelarrowexporter] Add internal/grpcutil package Jul 23, 2024
@jmacd
Copy link
Contributor Author

jmacd commented Jul 24, 2024

🥺 I think this could merge?

@mwear mwear added the ready to merge Code review completed; ready to merge by maintainers label Jul 26, 2024
@jmacd
Copy link
Contributor Author

jmacd commented Jul 30, 2024

Please merge this.

@jmacd
Copy link
Contributor Author

jmacd commented Jul 31, 2024

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

Copy link
Contributor

@evan-bradley evan-bradley left a 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.

internal/grpcutil/timeout.go Show resolved Hide resolved
@mx-psi
Copy link
Member

mx-psi commented Aug 6, 2024

@evan-bradley is this good to merge?

@evan-bradley evan-bradley merged commit 094f95e into open-telemetry:main Aug 6, 2024
154 checks passed
@github-actions github-actions bot added this to the next release milestone Aug 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants