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.

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

add debug exporter #8375

Closed
wants to merge 4 commits into from
Closed

add debug exporter #8375

wants to merge 4 commits into from

Conversation

codeboten
Copy link
Contributor

This PR does the following:

  • creates a new debugexporter that instantiates the loggingexporter initially
  • adds a method in the logging exporter to make it possible for the debug exporter to instantiate it w/ a new name
  • adds a deprecation notice to loggingexporter
  • updates documentation + examples to use debug instead of logging

I would suggest to move forward w/ this, we could release v0.85.0 w/ both exporters.

FIxes #7769

@codeboten codeboten requested review from a team and jpkrohling September 7, 2023 17:41
@codeboten codeboten changed the title Codeboten/debug exporter add debug exporter Sep 7, 2023
@codeboten codeboten mentioned this pull request Sep 7, 2023
Closed
@codeboten
Copy link
Contributor Author

Pinging @jpkrohling as you'd reviewed the PR this replaces

@codeboten codeboten force-pushed the codeboten/debug-exporter branch from 4be8194 to 8053951 Compare September 7, 2023 18:49
@codecov
Copy link

codecov bot commented Sep 7, 2023 •
edited
Loading

Codecov Report

Patch coverage is 63.63% of modified lines.

❗ Current head 2243391 differs from pull request most recent head 343db30. Consider uploading reports for the commit 343db30 to get more accurate results

Files Changed Coverage
client/client.go ø
exporter/loggingexporter/factory.go 50.00%
cmd/otelcorecol/components.go 100.00%
exporter/debugexporter/factory.go 100.00%

📢 Thoughts on this report? Let us know!.

Alex Boten added 3 commits September 7, 2023 15:55
[wip] prototype debug exporter
7ba6025 
Signed-off-by: Alex Boten <aboten@lightstep.com>
fix integration test
d04b7c1 
Signed-off-by: Alex Boten <aboten@lightstep.com>
remove unused vars
2243391 
Signed-off-by: Alex Boten <aboten@lightstep.com>
@codeboten codeboten force-pushed the codeboten/debug-exporter branch from cd4bddd to 2243391 Compare September 7, 2023 22:56
jpkrohling
jpkrohling approved these changes Sep 8, 2023
View reviewed changes
Copy link
Member

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

LGTM. A minor comment on how to migrate from one component to another, but it can be done in a follow-up PR.

exporter/loggingexporter/README.md Outdated Show resolved Hide resolved
dmitryax
dmitryax reviewed Sep 8, 2023
View reviewed changes
exporter/loggingexporter/factory.go
func NewFactoryWithName(s component.Type) exporter.Factory {
return exporter.NewFactory(
s,
createDefaultConfig,
Copy link
Member

@dmitryax dmitryax Sep 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Instead of introducing the exported function NewFactoryWithName and keep it for a year, maybe we can move createDefaultConfig to exporter/internal/ package and have both debug and logging exporters use it?

It probably should be done along with Config struct. logging and debug exporter should have aliases to that Config

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me try that. I'll move this PR back to draft while I try this out.

@dmitryax
Copy link
Member

dmitryax commented Sep 8, 2023 •
edited
Loading

The loggingexporter module has a public type Config, which the debug exporter doesn't have yet. This made me think that moving all the code to the new module is still worth doing after this PR. I believe it's better to have all the code in the new module to avoid making changes to the deprecated one.

@codeboten codeboten marked this pull request as draft September 8, 2023 19:17
@codeboten codeboten mentioned this pull request Sep 8, 2023
Merged
codeboten pushed a commit that referenced this pull request Sep 15, 2023
add debug exporter, move code from logging exporter into internal (#8378
456e75c 
)

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

---------

Signed-off-by: Alex Boten <aboten@lightstep.com>
@codeboten
Copy link
Contributor Author

Superseeded by #8378

@codeboten codeboten closed this Sep 15, 2023
@codeboten codeboten deleted the codeboten/debug-exporter branch September 15, 2023 20:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dmitryax dmitryax dmitryax left review comments

@jpkrohling jpkrohling jpkrohling approved these changes

@andrzej-stencel andrzej-stencel Awaiting requested review from andrzej-stencel andrzej-stencel will be requested when the pull request is marked ready for review andrzej-stencel is a code owner

@mx-psi mx-psi Awaiting requested review from mx-psi mx-psi will be requested when the pull request is marked ready for review mx-psi is a code owner

@evan-bradley evan-bradley Awaiting requested review from evan-bradley evan-bradley will be requested when the pull request is marked ready for review evan-bradley is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Should the logging exporter be renamed?
3 participants
@codeboten @dmitryax @jpkrohling