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

Add internal/otelarrow package for shared code between otelarrow exporter, receiver #33579

Merged
merged 32 commits into from
Jul 17, 2024

Conversation

jmacd
Copy link
Contributor

@jmacd jmacd commented Jun 14, 2024

Description: The otelarrowexporter and otelarrowreceiver components depend on several libraries in the open-telemetry/otel-arrow repository, which would complicate release management in both repositories. This change eliminates the dependency on otel-arrow/collector by moving each of those packages into a subdirectory of internal/otelarrow.

Link to tracking Issue: #33567

Testing: This brings new integration tests into this repository for the two otelarrow components.

Documentation: Each of the three non-test packages exposed here has a brief README.md explaining why it exists. Each of these could be useful to non-Arrow components if helpful.

@jmacd jmacd changed the title Jmacd/internalarrow Add internal/otelarrow package for shared code between otelarrow exporter, receiver Jun 14, 2024
@jmacd jmacd marked this pull request as ready for review June 17, 2024 17:06
@jmacd jmacd requested a review from a team June 17, 2024 17:07
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.

LGTM

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

Reviewers -- this is not stale. This PR is blocking future development and minor improvements of the otel-arrow components. This is also intended to avoid dependency skew between these packages (which depend on unstable collector APIs but reside in an external repository) before they begin to hold up releases in this repository.

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. One non-blocking question and a couple of suggestions.

internal/otelarrow/compression/zstd/mru.go Show resolved Hide resolved
internal/otelarrow/netstats/README.md Outdated Show resolved Hide resolved
internal/otelarrow/netstats/README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

Opened a follow up issue to move the usage of otel metrics to mdatagen #34135

@codeboten codeboten merged commit 7e3fc98 into open-telemetry:main Jul 17, 2024
154 checks passed
@github-actions github-actions bot added this to the next release milestone Jul 17, 2024
evan-bradley pushed a commit that referenced this pull request Aug 6, 2024
…33688)

**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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants