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

[exporter/sumologicexporter] Initialized sender to be reused instead of creating it for each batch #33643

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

chan-tim-sumo
Copy link
Contributor

Description:
Refactored sender to be created once and reused instead of creating a new sender each batch.

Link to tracking Issue:

Testing:

  • Unit test passed

Documentation:

  • chloggen

@sumo-drosiek
Copy link
Member

We do not need changelog for this change. It doesn't change neither api nor behavior

@@ -146,7 +147,27 @@ func newTracesExporter(
// start starts the exporter
func (se *sumologicexporter) start(ctx context.Context, host component.Host) (err error) {
se.host = host
return se.configure(ctx)
err = se.configure(ctx)

Choose a reason for hiding this comment

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

I think we need to recreate the sender inside configure. configure may be called asynchronously as a result of receiving an unauthorized error from the remote, in which case we want to get the data urls again. This used to be done implicitly by creating a new sender, but with this change we need to do so explicitly and handle any resultant concurrency issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes sense 💪

@chan-tim-sumo
Copy link
Contributor Author

We do not need changelog for this change. It doesn't change neither api nor behavior

seems to have a build error without a changelog 🤔

@sumo-drosiek
Copy link
Member

seems to have a build error without a changelog 🤔

@jpkrohling Could you add label to skip the changelog check please?

@chan-tim-sumo
Copy link
Contributor Author

hey @songy23 can you take a look at this pr? and if you're able to, can you add label to skip the changelog check? thanks alot!

Copy link
Member

@songy23 songy23 left a comment

Choose a reason for hiding this comment

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

LGTM

@songy23
Copy link
Member

songy23 commented Jun 26, 2024

Looks like you have a changelog already so I don't think Skip changelog is needed

or if you prefer to not have the changelog, please remove it from this PR

@chan-tim-sumo
Copy link
Contributor Author

chan-tim-sumo commented Jun 26, 2024

Looks like you have a changelog already so I don't think Skip changelog is needed

or if you prefer to not have the changelog, please remove it from this PR

Gotcha @songy23 , added it before since build failed without it. But just removed the change log.

@songy23 songy23 added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Jun 26, 2024
@chan-tim-sumo
Copy link
Contributor Author

hey @dmitryax , can you take a look at this pr too? thanks alot! 💪

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
exporter/sumologic Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants