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 #33047

Closed

Conversation

adcharre
Copy link
Contributor

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.

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.

adcharre added 2 commits May 13, 2024 19:41
Use OpAMP custom messages to report progress of start/end time based ingest from an S3 bucket.
@adcharre adcharre requested a review from atoulme as a code owner May 14, 2024 14:29
@adcharre adcharre requested a review from a team May 14, 2024 14:29
component: awss3receiver

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: 'Add support for monitoring the progress of ingesting data from an S3 bucket via OpAMP custom messages.'
Copy link
Contributor

Choose a reason for hiding this comment

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

wow. That's the first time I see this done. Can you use obsreport to report metrics first?

| `stop_time` | The time to stop retrieving data in RFC3339 format. |
| `ingest_time` | The time of the data currently being ingested in RFC3339 format. |
| `failure_message` | Error message if `ingest_status` is "failed". |
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a spec for this as part of opAmp or maybe entities?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no spec for this and that is the reason why I'm using a Custom Capability.

return nil
}

func (n *opampNotifier) Shutdown(_ context.Context) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are you exporting Shutdown, Start and SendStatus? Are all 3 part of an interface?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are part of the statusNotifier interface defined in the same file. Possibly overkill at this stage, I was allowing for other methods of notification (ie Webhook, file ...)

@atoulme
Copy link
Contributor

atoulme commented May 14, 2024

I believe it is worth a good SIG convo:
When is that advisable?
Looks like you'd like metrics for this, right?
Or maybe even traces?
Is opAmp the place to send notifications too anyway? Is there a spec, is there something else here I'm missing? Can we do this on a more established component first to test it out with more eyeballs?

@atoulme atoulme added the discussion needed Community discussion needed label May 14, 2024
@tigrannajaryan
Copy link
Member

I want to echo @atoulme's questions. This can be done via OpAMP, but I wonder if it should be done via OpAMP. Can the receiver generate metrics or traces that describe what happens instead? Is there a reason it has to go via OpAMP?

I think this is something for Collector maintainers to decide: how should components report progress details of what they do.

One thing to watch out for is that OpAMP has no built-in concept of batching. If the rate of custom messages is very high it may turn out to be inefficient (probably more so for plain http transport than for WS transport).

@adcharre
Copy link
Contributor Author

@atoulme / @tigrannajaryan - Fair questions - though I'm not looking for metrics as the intention is to use the collector as one component of a larger app to retrieve data from S3 back into an observability platform. As a result I'm looking at being able to report progress to a user so they know when the requested trace has been ingested.

I'll look at adding obsreport metrics as suggested while this is up for discussion.

@tigrannajaryan
Copy link
Member

@adcharre it would be great to learn more about your use case and perhaps OpAMP is the right solution for it.

@atoulme
Copy link
Contributor

atoulme commented May 14, 2024

We're discussing this here as well: https://cloud-native.slack.com/archives/C02J58HR58R/p1715714974462069

Comment on lines +68 to +70
the time of the data being ingested when the failure occurred.
If the ingest process completes successfully a status message with `ingest_status` set to "completed" is sent.
Copy link
Contributor

Choose a reason for hiding this comment

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

Docs writer here. Just a quick edit for clarity. Thanks!

Suggested change
If during the processing of the data an error occurs a status message with `ingest_status` set to "failed" status with
the time of the data being ingested when the failure occurred.
If the ingest process completes successfully a status message with `ingest_status` set to "completed" is sent.
If an error occurs during the processing of the data, a status message with `ingest_status` set to "failed" is sent, along with the time the ingestion failure occurred.
If the ingest process completes successfully, a status message with `ingest_status` set to "completed" is sent.

Copy link
Contributor

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 May 29, 2024
Copy link
Contributor

Closed as inactive. Feel free to reopen if this PR is still being worked on.

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.

5 participants