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

Create a skeleton for the 'blobuploadprocessor'. #35966

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

michaelsafyan
Copy link
Contributor

@michaelsafyan michaelsafyan commented Oct 23, 2024

Description

Create a skeleton for the blobuploadprocessor component.

A more fleshed out implementation exists in:

However, the size of the entire thing is a bit unwieldy and likely to become a barrier for an effective code review.

In order to make progress upstreaming this and to ensure a reasonable implementation path, I'm now trying to break out little bits of the implementation into separate, smaller, more focused PRs. And, in the process of this, attempting to simplify the code, improve testing, etc. over the reference proof-of-concept.

Link to tracking issue

#33737 : "New component: Blob Upload Processor"

Testing

Contains a "smoke test" in package_test.go that validates the basic contract for the connector and ensures non-crashing.

Also contains a few other unit tests and placeholders for additional unit tests.

As actual implementation pieces land in subsequent PRs, we will add additional corresponding tests.

Documentation

Contains a README.md that explains the purpose of this component and its intended trajectory.

Also contains a metadata.yaml providing structured documentation for the component.

We will update the README.md document as additional implementation lands to keep it up-to-date with actual state.

@github-actions github-actions bot added the cmd/otelcontribcol otelcontribcol command label Oct 23, 2024
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.

Pinging @dashpole as the sponsor for this component

@dashpole
Copy link
Contributor

Ack. My availability is limited until after kubecon, but I will try to get to this when I can.

@dashpole dashpole self-assigned this Oct 30, 2024
@michaelsafyan michaelsafyan changed the title Create a skeleton for the 'blobuploadconnector'. Create a skeleton for the 'blobuploadprocessor'. Nov 4, 2024
Copy link
Contributor

@dashpole dashpole left a comment

Choose a reason for hiding this comment

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

looking good. Just small tweaks

note: Processor for uploading pieces of a signal to blob storage and replacing with a ref
issues: [33737]
subtext: Skeleton implementation at the moment.
change_logs: []
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
change_logs: []
change_logs: [api, user]

@@ -20,6 +20,7 @@ body:
- confmap/provider/aesprovider
- confmap/provider/s3provider
- confmap/provider/secretsmanagerprovider
- connector/blobupload
Copy link
Contributor

Choose a reason for hiding this comment

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

update to processor

@@ -20,6 +20,7 @@ body:
- confmap/provider/aesprovider
- confmap/provider/s3provider
- confmap/provider/secretsmanagerprovider
- connector/blobupload
Copy link
Contributor

Choose a reason for hiding this comment

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

update to processor

@@ -25,6 +25,7 @@ body:
- confmap/provider/aesprovider
- confmap/provider/s3provider
- confmap/provider/secretsmanagerprovider
- connector/blobupload
Copy link
Contributor

Choose a reason for hiding this comment

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

update to processor

@@ -0,0 +1,96 @@
# Blob Upload Connector
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# Blob Upload Connector
# Blob Upload Processor

"github.com/open-telemetry/opentelemetry-collector-contrib/processor/blobuploadprocessor/internal/metadata"
)

// NewFactoryWithDeps instantiates the factory with dependency injection, allowing
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// NewFactoryWithDeps instantiates the factory with dependency injection, allowing
// newFactoryWithDeps instantiates the factory with dependency injection, allowing

# Implementation still in-progress. Please follow along at:
# https://github.com/open-telemetry/opentelemetry-collector-contrib/issues/33737
development: [traces, logs]
distributions: [contrib]
Copy link
Contributor

Choose a reason for hiding this comment

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

adding this to contrib is done after the implementation is complete (it reaches alpha).

Suggested change
distributions: [contrib]
distributions: []

skip: false
ignore:
# See https://github.com/census-instrumentation/opencensus-go/issues/1191 for more information.
top: go.opencensus.io/stats/view.(*worker).start
Copy link
Contributor

Choose a reason for hiding this comment

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

Strange that this is required given opencensus isn't in the go.sum. We can try removing it in a follow-up.

@michaelsafyan michaelsafyan marked this pull request as draft November 21, 2024 16:11
Copy link
Contributor

github-actions bot commented Dec 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 Dec 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cmd/otelcontribcol otelcontribcol command Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants