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

Should the logging exporter be renamed? #7769

Closed
codeboten opened this issue May 24, 2023 · 17 comments · Fixed by #8378
Closed

Should the logging exporter be renamed? #7769

codeboten opened this issue May 24, 2023 · 17 comments · Fixed by #8378
Labels
bug Something isn't working exporter/logging Logging exporter related issues

Comments

@codeboten
Copy link
Contributor

codeboten commented May 24, 2023

Please vote in the comment below #7769 (comment)

@codeboten codeboten added the bug Something isn't working label May 24, 2023
@bogdandrutu
Copy link
Member

Please call it "debug" :)

@atoulme
Copy link
Contributor

atoulme commented May 24, 2023

Let's rename it before we mark the component away from development - See #7768

@atoulme
Copy link
Contributor

atoulme commented May 24, 2023

If we rename, let's keep the existing name for at least 3 releases and offer a deprecation warning, this is used everywhere.

@codeboten
Copy link
Contributor Author

Fwiw, we moved away from from loglevel back in october and users are still running into issues w/ this due to documentation floating around that uses the incorrect config. 3 releases may not be enough for this change

@atoulme
Copy link
Contributor

atoulme commented May 24, 2023

Agree, let's use a year time then.

@andrzej-stencel
Copy link
Member

I agree logging is not a good name for this exporter. This issue has been raised before, last time in Collector SIG meeting on December 21, 2022. Here are two related issues:

If we do decide to rename, does it mean that rather than strictly renaming, we would need to create a new exporter under a different name that would be a copy of the current logging exporter, and deprecate the logging exporter? Possibly keeping the deprecated one "forever", or at least for a year.

@codeboten
Copy link
Contributor Author

@astencel-sumo we could add a method to the logging exporter to make it instantiable under a different configuration option, something like #7773

@mx-psi
Copy link
Member

mx-psi commented May 26, 2023

This seems like another use case for #7631.

One question that comes to mind is how are ocb custom build users going to be notified of the change. Here are some possible approaches:

  1. Deprecate the go.opentelemetry.io/collector/exporter/loggingexporter module and add some mechanism in ocb to notify users about deprecated modules
  2. Handle this as a special case in the ocb code and add the debug exporter to builds if the logging exporter is present.
  3. Handle this as a special case in the core Collector code instead and add the debug exporter to the list of exporters if the logging exporter is present.

I ordered them from the one I like the most to the one I like the least.

@DewaldDeJager
Copy link

DewaldDeJager commented Jun 7, 2023

  1. Deprecate the go.opentelemetry.io/collector/exporter/loggingexporter module and add some mechanism in ocb to notify users about deprecated modules

I agree with this approach. I'm not sure I agree with wrapping one exporter from the other (as per discussions in the SIG meeting). The deprecated component should have no further development and users should be nudged to switch to the new component. The downside of this is that the code will be duplicated initially.

I also agree that it's a good idea for ocb to log warnings when trying to include any deprecated modules so that the distribution maintainers can look at replacing it. We should consider logging a warning about any deprecated components when the Collector starts if we don't have that already. This will help the distribution maintainers as their end-users will be nudged to move over to the new component.

@codeboten
Copy link
Contributor Author

@open-telemetry/collector-approvers @open-telemetry/collector-contrib-approvers is debug the name we want to go with?

Looking through various otel repos, there's a few different names already for this type of exporter: stdout (https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk_exporters/stdout.md) & console (https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/src/OpenTelemetry.Exporter.Console/README.md) come to mind.

@TylerHelmuth
Copy link
Member

TylerHelmuth commented Jun 15, 2023

@codeboten what problem does renaming the exporter solve? Are users getting confused thinking that the exporter write to a log? The issues @astencel-sumo linked are valid, but were not raised by end users. I am curious if we are proposing this because we don't like the name or if it is hurting users.

@codeboten
Copy link
Contributor Author

This issue was raised as a result of the discussion in the SIG call 3 weeks ago to decide whether or not to rename the exporter. Based on the history in #3524, I would say we should not to this, specifically the comment here: #3609 (review)

My vague memory from the discussion in the call was whether we need to rename it before changing its status. If we agree that logging is good enough, then we can avoid doing the work here.

@jpkrohling
Copy link
Member

jpkrohling commented Jun 15, 2023

Logging is confusing as, in the context of the collector, we have a logs pipeline and a logging exporter. I've seen users confused by that. During the SIG call, I remember folks also wanting to get compatibility rules for the output being generated by the logging exporter, meaning that they intend to rely on that, and they really shouldn't.

Renaming it to debug will make it clear what the exporter really is about.

That said, stdout is used in SDKs, so, perhaps that's a better name.

@mx-psi
Copy link
Member

mx-psi commented Jun 16, 2023

If we are going to rename it, I would go with debug. But to be frank, I don't see a future where we can get rid of the logging name with the tools we have today, the experience with #6334 shows that our users take a significant amount of time to update and adapt to deprecations. I think realistically the question is: are we willing to have to maintain compatibility with both names forever to do this change?

Personally, while I appreciate the arguments being made, I am starting to think that it's better if we keep things as is and don't do the renaming: it seems like the price to pay is too high.

@andrzej-stencel
Copy link
Member

andrzej-stencel commented Jun 20, 2023

If we are going to rename it, I would go with debug.

I second that. If I saw a stdout exporter, I would think "oh, so like file exporter, but to stdout". If we make this change, we want to emphasize that this is a debugging tool, not a tool to export logs to a console or to stdout in a format anyone might want to rely on.

I agree this is a change that's easy to dismiss as it is not a "feature" and it carries a lot of cost.

At the same time, I believe that good names make a lot of difference. They make products more user friendly, especially to new users. Confusing names (can we agree that logging is a confusing name?) make onboarding harder and add to the cognitive load, making the learning curve steeper. The more quirks like this, the less pleasant it is to use the product also in the long run.

My recommendation is to mark the logging exporter as deprecated and leave it there for a year, maybe two years. I would copy its code and create a separate debug exporter that can then be independently developed, without making any changes to the deprecated code.

This will take a lot of time, but in two years, everyone will be hopefully using the debug exporter and the world will be a tiny bit less awful better. 😉

@codeboten
Copy link
Contributor Author

codeboten commented Jun 21, 2023

Maybe voting on this is the best way to move forward? It feels like we could be stuck in this discussion for a long time without anything actionable resulting from it :) I would like to call out that at least one other language (java) calls the debug/stdout exporter a logging exporter as per the issue in the spec here.

What should we do with this exporter?

🚀 nothing, leave the loggingexporter as is and close this issue as a reference of the decision
❤️ rename to debugexporter and mark the logging exporter as deprecated for 1 year
👀 improve the loggingexporter to support additional functionality like allowing users to decide on the output format

@jmacd
Copy link
Contributor

jmacd commented Jun 21, 2023

E.g., of additional functionality in the contrib fileexporter open-telemetry/opentelemetry-collector-contrib#22079

@andrzej-stencel andrzej-stencel added the exporter/logging Logging exporter related issues label Jun 27, 2023
codeboten pushed a commit that referenced this issue Sep 15, 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

---------

Signed-off-by: Alex Boten <aboten@lightstep.com>
mx-psi pushed a commit that referenced this issue Oct 3, 2024
#### Description

It is September, which means we get to remove this deprecated component.

- The helm chart was updated to exclude the logging exporter already:
https://github.com/open-telemetry/opentelemetry-helm-charts/blob/main/charts/opentelemetry-collector/UPGRADING.md#0840-to-0850.
- The Operator references have been updated:
open-telemetry/opentelemetry-operator#3259.
- Opentelemetry.io site final references to update:
open-telemetry/opentelemetry.io#5143

<!-- Issue number if applicable -->
#### Link to tracking issue
Related to
#7769
jackgopack4 pushed a commit to jackgopack4/opentelemetry-collector that referenced this issue Oct 8, 2024
#### Description

It is September, which means we get to remove this deprecated component.

- The helm chart was updated to exclude the logging exporter already:
https://github.com/open-telemetry/opentelemetry-helm-charts/blob/main/charts/opentelemetry-collector/UPGRADING.md#0840-to-0850.
- The Operator references have been updated:
open-telemetry/opentelemetry-operator#3259.
- Opentelemetry.io site final references to update:
open-telemetry/opentelemetry.io#5143

<!-- Issue number if applicable -->
#### Link to tracking issue
Related to
open-telemetry#7769
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working exporter/logging Logging exporter related issues
Projects
None yet
9 participants