Add 'debug' exporter #7773
Add 'debug' exporter #7773
Conversation
There was a problem hiding this comment.
Alternative, implement a https://github.com/open-telemetry/opentelemetry-collector/blob/main/confmap/converter.go#L12 that converts the config exporters::logging[/.*]? to exporters::debug[/.*]?. Then we move the code to debug.
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
There was a problem hiding this comment.
Alternative, implement a https://github.com/open-telemetry/opentelemetry-collector/blob/main/confmap/converter.go#L12 that converts the config exporters::logging[/.]? to exporters::debug[/.]?. Then we move the code to debug.
@bogdandrutu Two questions around this:
- Would you end up creating a copy of the entire config map to do the replace? I couldn't find a way to delete a key other than by creating another map and omitting it
- This would also require changing any
logging/*in the configured pipelines, would you still want to go this route?
codeboten
force-pushed
the
codeboten/debug-exporter
branch
from
4334d25 to
51cca95
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #7773 +/- ##
==========================================
+ Coverage 90.79% 91.05% +0.26%
==========================================
Files 300 307 +7
Lines 14983 15389 +406
==========================================
+ Hits 13604 14013 +409
+ Misses 1103 1097 -6
- Partials 276 279 +3
☔ View full report in Codecov by Sentry. |
codeboten
force-pushed
the
codeboten/debug-exporter
branch
from
c9cddb8 to
acf68d9
Compare
| @@ -4,6 +4,7 @@ go 1.19 | |||
|
|
|||
| require ( | |||
| github.com/stretchr/testify v1.8.4 | |||
| go.opentelemetry.io/collector v0.80.0 | |||
There was a problem hiding this comment.
i ran make gotidy and this happened
| @@ -161,21 +161,21 @@ $ curl -X POST localhost:9411/api/v2/spans -H'Content-Type: application/json' -d | |||
| You should see a log entry like the following from the Collector: | |||
|
|
|||
| ``` | |||
| 2020-11-11T04:12:33.089Z INFO loggingexporter/logging_exporter.go:296 TraceExporter {"#spans": 1} | |||
| 2020-11-11T04:12:33.089Z INFO debugexporter/debug_exporter.go:296 TraceExporter {"#spans": 1} | |||
There was a problem hiding this comment.
Call me paranoid, but can you bump the date to a date closer to today? It might be just me, but I would be really confused seeing this timestamp and no versions of the collector around that date having this component.
| exporter.WithTraces(createTracesExporter, component.StabilityLevelDevelopment), | ||
| exporter.WithMetrics(createMetricsExporter, component.StabilityLevelDevelopment), | ||
| exporter.WithLogs(createLogsExporter, component.StabilityLevelDevelopment), | ||
| exporter.WithTraces(createTracesExporter, component.StabilityLevelDeprecated), |
There was a problem hiding this comment.
I think the logging exporter will live on for quite a while, right? Would it make sense to get rid of the code for this exporter and embed the new exporter here? You could probably just return the debug exporter's outcome to NewFactory here, no?
There was a problem hiding this comment.
We'll need to release a new version of the exporter to mark it as "deprecated", so i figured i would remove the code on the next release.
I would remove the code completely and continue to include the module in the built distros, WDYT?
There was a problem hiding this comment.
I would remove the code completely and continue to include the module in the built distros, WDYT?
Can you expand on that? Are you going to keep including the same version of the logging exporter? I don't think that's what you meant, but it's not clear to me what the plan is.
There was a problem hiding this comment.
keep including the same version of the logging exporter?
This would be one option.
Another would be to go w/ the wrapper approach, i originally proposed this. A third option would be to do what @bogdandrutu suggested here and implement a converter.
I could go with any of those, but either ways we'll need both modules published for at least one version to deprecate the logging exporter
There was a problem hiding this comment.
Another would be to go w/ the wrapper approach
That's what I also had in mind as well, I think it's the cleanest. The new exporter gets the code, the old exporter wraps the new exporter, so that it's code base is minimal and can continue to exist with almost no maintenance for a year before we delete it from the code base.
we'll need both modules published for at least one version to deprecate the logging exporter
If the cut off date is July 2024, we should continue publishing new versions of the logging exporter until then. Wrapping the debug exporter within the logging exporter allows that with minimal overhead.
There was a problem hiding this comment.
Finally getting back to this PR. Updated it to follow your suggestion, PTAL
Signed-off-by: Alex Boten <[email protected]>
Signed-off-by: Alex Boten <[email protected]>
Signed-off-by: Alex Boten <[email protected]>
Signed-off-by: Alex Boten <[email protected]>
Signed-off-by: Alex Boten <[email protected]>
Signed-off-by: Alex Boten <[email protected]>
Signed-off-by: Alex Boten <[email protected]>
codeboten
force-pushed
the
codeboten/debug-exporter
branch
from
f8563c6 to
fe15c5c
Compare
Signed-off-by: Alex Boten <[email protected]>
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
|
Reopening this PR. Followed the suggestion from @jpkrohling on aliasing the logging exporter as a debug exporter |
|
Well apparently github didn't like me trying to re-open this PR so i opened a new one here: #8375 |
This PR does the following:
debuginstead ofloggingI would suggest to move forward w/ this, we could release v0.81.0 w/ both exporters, and then as of v0.82.0, we could only release the debug exporter, but including the logging exporter at v0.81.0 in the release file. Any thoughts?
FIxes #7769