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

add debug exporter, move code from logging exporter into internal #8378

Merged

Conversation

codeboten
Copy link
Contributor

@codeboten codeboten commented Sep 8, 2023

Alternative to #8375

This moves most of the code from the logging exporter into two internal packages that will eventually end up in the debugexporter once the loggingexporter has been removed.

Fixes #7769

@codeboten
Copy link
Contributor Author

@dmitryax @jpkrohling please have a look at this PR as an alternative to "aliasing" the logging exporter.

Pros:

  • not causing the debug exporter to depend on the logging exporter
  • no longer adds a new public method to the logging exporter.

Cons:

  • code that was internal to the logging exporter is now in an exporter/internal package
  • there is some amount of duplication in the code

exporter/debugexporter/config.go Show resolved Hide resolved
exporter/debugexporter/factory.go Outdated Show resolved Hide resolved
"go.opentelemetry.io/collector/pdata/ptrace"
)

type debugExporter struct {
Copy link
Member

Choose a reason for hiding this comment

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

Why can't we move this one to internal?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could, it just didn't seem to really fit, i guess i could create an exporter/internal/loggingcommonexporter package or something like it

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, something like that. I'm just thinking of how we can reduce the code duplication. Looks like it's technically possible. If so, I'd merge this PR and do that later rather than #8375

@jpkrohling
Copy link
Member

Is this to address this comment?

I believe it's better to have all the code in the new module to avoid making changes to the deprecated one.

If so, wouldn't it be sufficient to move the code from logging to debugging, keeping logging as the wrapper to the new one?

@codeboten
Copy link
Contributor Author

  • not causing the debug exporter to depend on the logging exporter
  • no longer adds a new public method to the logging exporter.

@jpkrohling the advantages are really not having to create a dependency between the two modules moving forward and avoiding having to add a public method. With the latest change, there's little code duplication. PTAL

@codeboten codeboten marked this pull request as ready for review September 13, 2023 00:55
@codeboten codeboten requested review from a team and dmitryax September 13, 2023 00:55
@jpkrohling
Copy link
Member

the advantages are really not having to create a dependency between the two modules moving forward

Is it a big problem? I mean, you are in the end creating the same dependency through a proxy, which is the common library, no? If you change the debug exporter, we need to either change the common library or fork the code, which is pretty much what we'd have to do if we had the logging exporter wrapping the debug exporter.

and avoiding having to add a public method

It was caused by the desire to share the createDefaultConfig implementation in the factory, right? It's such a small function. Wouldn't it be worth just having two implementations of it and of the factory? The logging exporter's factory could just use the debug exporter's factory. Wouldn't it work?

@codeboten codeboten force-pushed the codeboten/add-debug-exporter-2 branch from 916e9ba to ddf1c6c Compare September 13, 2023 21:26
@codeboten codeboten changed the title add debug exporter as mostly a copy of logging exporter add debug exporter, move code from logging exporter into internal Sep 14, 2023
@codeboten codeboten force-pushed the codeboten/add-debug-exporter-2 branch from 3c1e873 to bc5f5e0 Compare September 14, 2023 21:40
cmd/builder/README.md Outdated Show resolved Hide resolved
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.

Should the logging exporter be renamed?
5 participants