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.

Sign up for GitHub

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

Jump to bottom

[exporterhelper] Refactor options and baseExporter #8369

Merged

dmitryax merged 1 commit into open-telemetry:main from dmitryax:refactor-exporter-sender

Sep 8, 2023

Member

dmitryax commented Sep 6, 2023 •

edited

Loading

Separate all the parts of the baseExporter into an explicit chain of request senders. It makes it easier to follow the data flow and add additional senders.

This change also removes the baseSettings, because keeping the settings is not needed anymore. All the options update the internal senders in place.

This change also removes confusing error messages like "Exporting failed. Dropping data. Try enabling sending_queue to survive temporary failures" when the Queue is not even available in the exporter (WithQueue option is not applied) which means users don't sending_queue config option. Now, such messages are only shown when the sending_queue (or retry_on_failure) is available, but not enabled by the user.

dmitryax force-pushed the refactor-exporter-sender branch 3 times, most recently from 7f46194 to 44c8eae Compare

September 7, 2023 19:13

codecov bot commented Sep 7, 2023 •

edited

Loading

Codecov Report

Patch coverage is 97.29% of modified lines.

Files Changed	Coverage
exporter/exporterhelper/common.go	`95.29%`
exporter/exporterhelper/logs.go	`100.00%`
exporter/exporterhelper/metrics.go	`100.00%`
exporter/exporterhelper/queued_retry.go	`100.00%`
exporter/exporterhelper/traces.go	`100.00%`

📢 Thoughts on this report? Let us know!.

dmitryax force-pushed the refactor-exporter-sender branch from 44c8eae to 672af7e Compare

September 7, 2023 19:43

dmitryax added the Skip Changelog label

dmitryax force-pushed the refactor-exporter-sender branch 2 times, most recently from 096111c to 7c5b10d Compare

September 7, 2023 19:51

dmitryax changed the title ~~[DRAFT] [exporterhelper] Refactor options and baseExporter~~ [chore] [exporterhelper] Refactor options and baseExporter

dmitryax marked this pull request as ready for review

September 7, 2023 19:52

dmitryax requested review from a team and codeboten

September 7, 2023 19:52

dmitryax commented

View reviewed changes

exporter/exporterhelper/common.go

               // Option apply changes to baseSettings.
-              type Option func(*baseSettings)
+              type Option func(*baseExporter)

Member Author

dmitryax Sep 7, 2023

Changing this signature should not be considered a breaking change since the argument is of unexported type, so users cannot implement it directly

Member

bogdandrutu Sep 7, 2023

would suggest in a separate PR to change this to an interface with a private func. I think this is kind of the recommended new way of doing options.

dmitryax force-pushed the refactor-exporter-sender branch 6 times, most recently from 73cbd59 to 2c33fb1 Compare

September 7, 2023 21:13

dmitryax removed the Skip Changelog label

dmitryax changed the title ~~[chore] [exporterhelper] Refactor options and baseExporter~~ [exporterhelper] Refactor options and baseExporter

dmitryax force-pushed the refactor-exporter-sender branch from 2c33fb1 to 28bc997 Compare

September 7, 2023 22:51

bogdandrutu reviewed

View reviewed changes

exporter/exporterhelper/common.go

               // Option apply changes to baseSettings.
-              type Option func(*baseSettings)
+              type Option func(*baseExporter)

Member

bogdandrutu Sep 7, 2023

would suggest in a separate PR to change this to an interface with a private func. I think this is kind of the recommended new way of doing options.

exporter/exporterhelper/common.go Outdated

Comment on lines 235 to 241

+              	// If no error then start the queueSender.
+              	return be.queueSender.start(ctx, host, be.set)
+              }
+              func (be *baseExporter) Shutdown(ctx context.Context) error {
+              	// First shutdown the retry sender
+              	be.retrySender.shutdown()
+              	// Then shutdown the queue sender
+              	be.queueSender.shutdown()

Member

bogdandrutu Sep 7, 2023

Random senders that you start/stop. If the interface is like that, you should call start on all. Also should it call start on the next sender itself and same for stop?

Member Author

dmitryax Sep 8, 2023

The shutdown sequence is a bit random for a reason. Ideally, it could've been the same sequence as in the pipelines, the same as the data flow: queueSender -> retrySender -> user's exporter. But we need to stop the retrySender first so it can push an unfinished request back to the queue.

The start sequence should be applied in reverse order.

exporter/exporterhelper/common.go

               	}
               	return req.Export(ctx)
               }
+              func createSampledLogger(logger *zap.Logger) *zap.Logger {

Member

bogdandrutu Sep 7, 2023

We need to revisit this, seems very arbitrary to have a sampler logger only for some cases. Maybe just better document as comment of the func why we have this and where should be used, but it is still random for me.

Member Author

dmitryax Sep 8, 2023 •

edited

Loading

I agree. We have another effort to address this #8134. I'm just moving it from one file to another as is

Contributor

antonjim-te Sep 8, 2023

Thank you @dmitryax ,
Right as part of #8134 we want to have a Sampled logger as part of the TelemetrySetting and use it when needed.

Please @bogdandrutu @dmitryax take a look at my PR #8134. Thanks :)

exporter/exporterhelper/common.go Outdated Show resolved Hide resolved

dmitryax force-pushed the refactor-exporter-sender branch from 28bc997 to c5c1e7c Compare

September 8, 2023 04:15


          [chore] [exporterhelper] Refactor options and baseExporter

224eb93

Separate all the parts of the baseExporter into an explicit chain of request senders. It makes it easier to follow the data flow and add additional senders. 

This change also removes the baseSettings, because it's not needed to keep the settings anymore. All the options are now applied on the baseExporter and update the internal senders in place.

dmitryax force-pushed the refactor-exporter-sender branch from c5c1e7c to 224eb93 Compare

September 8, 2023 04:26

bogdandrutu approved these changes

View reviewed changes

dmitryax merged commit 4e71dc1 into open-telemetry:main

30 checks passed

github-actions bot added this to the next release milestone

dmitryax deleted the refactor-exporter-sender branch

September 11, 2023 00:37

dmitryax mentioned this pull request

[chore] [exporterhelper] Split exporter senders into different files #8398

Merged

dmitryax added a commit that referenced this pull request


          [chore] [exporterhelper] Split exporter senders into different files (#…

767b95e

…8398)

This is a follow-up to
#8369. No
functional changes, just moving the code around

This was referenced Jan 4, 2024

Miss error logs after changes in retry open-telemetry/opentelemetry-collector-contrib#30271

Closed

Terminal errors are not being logged by the expoterhelper in case queued_retry is not used #9219

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet