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

modify code to make it extensive for logs #42

Closed
wants to merge 1 commit into from
Closed

Conversation

narcis96
Copy link

@narcis96 narcis96 commented Sep 3, 2024

No description provided.

@narcis96 narcis96 force-pushed the narcis/add_logs branch 6 times, most recently from 302b3df to a6833fe Compare September 3, 2024 11:33
Copy link

@heitorganzeli heitorganzeli left a comment

Choose a reason for hiding this comment

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

Thanks for implementing the new PR! I think main concern is to isolate both component so that they could be used separately

@@ -60,7 +60,7 @@ The following settings are optional:

```yaml
receivers:
huaweicloudcesreceiver:
huaweicloudreceiver:

Choose a reason for hiding this comment

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

since LTS is effectively a different service, i think it makes more sense to keep separate components like awscloudwatchlogsexporter and awskinesisexporter

but, i'm open for discussion

return newHuaweiCloudReceiver(settings, Metrics, cfg, &metricsReceiver{consumer: metrics})
}

type logsReceiver struct {

Choose a reason for hiding this comment

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

if keeping the same component, i think you could separate files, ces_consumer, lts_consumer. And the same for the other files, so that they don't grow indefinitely

// Check https://github.com/vektra/mockery on how to install it on your machine.
//
//go:generate mockery --name LtsClient --case=underscore --output=./mocks
type LtsClient interface {

Choose a reason for hiding this comment

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

same for CES Client.

shouldn't it be enough to only have the methods used in our code?

Copy link
Author

Choose a reason for hiding this comment

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

You right. See #43

if err := rcvr.dataProcessor.processReceivedData(ctx, otpMetrics); err != nil {
return err
}
case Logs:

Choose a reason for hiding this comment

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

in my mind these methods should be independent. for example, someone could configure LTS receiver frequency with a higher frequency than metrics because so that less data is accumulated in each retrieval round

@narcis96
Copy link
Author

this PR is deprecated

@narcis96 narcis96 closed this Oct 30, 2024
@narcis96 narcis96 deleted the narcis/add_logs branch November 7, 2024 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants