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

[receiver/awss3receiver]: Add ingest progress notifications via OpAMP #33980

Open
wants to merge 66 commits into
base: main
Choose a base branch
from

Conversation

adcharre
Copy link
Contributor

@adcharre adcharre commented Jul 9, 2024

Description: Add support for monitoring the progress of ingesting data from an S3 bucket via OpAMP custom messages.
This change would allow an appropriate UI to display the state of ingested.

This is a re-working of #33047 to use OTLP structures instead of a custom JSON object following discussions here: https://cloud-native.slack.com/archives/C02J58HR58R/p1715714974462069

Link to tracking Issue: #30750

Testing: Additional unit tests added. Full stack testing using a built collector and the example OpAMP server provided in the PR.

Documentation: README.md updated to include details of the configuration changes and details of the OpAMP Custom Capability and message format.

@atoulme
Copy link
Contributor

atoulme commented Jul 9, 2024

Please fix the conflict. Please have @evan-bradley and @tigrannajaryan review as this involves opAmp.

@adcharre
Copy link
Contributor Author

@evan-bradley - I've moved the example out to the examples/ directory which hopefully helps. Are you now happy enough to approve from your side?

Copy link
Contributor

@BinaryFissionGames BinaryFissionGames left a comment

Choose a reason for hiding this comment

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

Sorry for taking so long to look at this. It looks good, just noticed a couple discrepancies in the README vs the code.

receiver/awss3receiver/README.md Outdated Show resolved Hide resolved
receiver/awss3receiver/README.md Outdated Show resolved Hide resolved
@adcharre
Copy link
Contributor Author

@evan-bradley / @BinaryFissionGames - thanks for the reviews. I've updated with all the comments now and removed the example for the moment as it's not clear to me where this should live - I may move this out into a separate repo.

@adcharre
Copy link
Contributor Author

@atoulme / @evan-bradley / @tigrannajaryan / @BinaryFissionGames - Can I get another review please!

@adcharre adcharre requested a review from a team as a code owner September 20, 2024 10:27
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