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

chore: add logging example #2962

Closed
wants to merge 5 commits into from
Closed

chore: add logging example #2962

wants to merge 5 commits into from

Conversation

lyl156
Copy link

@lyl156 lyl156 commented Jun 19, 2022

It is PR for issue#2238.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jun 19, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

Copy link
Author

@lyl156 lyl156 left a comment

Choose a reason for hiding this comment

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

CI is not clear now.

Should I pull this and re-submit again?
Or is there something I can do to clear the CI?

@hanyuancheung
Copy link
Member

CI is not clear now.

Should I pull this and re-submit again? Or is there something I can do to clear the CI?

rerun make precommit command before committing the code.

@lyl156
Copy link
Author

lyl156 commented Jun 21, 2022

CI is not clear now.
Should I pull this and re-submit again? Or is there something I can do to clear the CI?

rerun make precommit command before committing the code.

I pull origin and make precommit.
However, the process fails at test/./exporters/jaeger every time. I am confused why this error occurs, because I didn't modify test/./exporters/jaeger.

FAIL    go.opentelemetry.io/otel/exporters/jaeger       0.325s
?       go.opentelemetry.io/otel/exporters/jaeger/internal/gen-go/agent [no test files]
?       go.opentelemetry.io/otel/exporters/jaeger/internal/gen-go/agent/agent-remote    [no test files]
?       go.opentelemetry.io/otel/exporters/jaeger/internal/gen-go/jaeger        [no test files]
?       go.opentelemetry.io/otel/exporters/jaeger/internal/gen-go/jaeger/collector-remote       [no test files]
?       go.opentelemetry.io/otel/exporters/jaeger/internal/gen-go/zipkincore    [no test files]
?       go.opentelemetry.io/otel/exporters/jaeger/internal/gen-go/zipkincore/zipkin_collector-remote    [no test files]
FAIL
make: *** [test/./exporters/jaeger] Error 1

@lyl156 lyl156 closed this Jun 21, 2022
@lyl156 lyl156 reopened this Jun 21, 2022
Copy link
Contributor

@MrAlias MrAlias left a comment

Choose a reason for hiding this comment

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

This PR adds an example of how to use the logging exporter for the collector. The linked issue this PR says it addresses is one that looks to refactor the existing collector example, not add another one with a different exporter. I do not see the value in accepting these changes, it does not add a unique example of how to use this project. I would recommend it is closed.

@lyl156
Copy link
Author

lyl156 commented Jun 21, 2022

This PR adds an example of how to use the logging exporter for the collector. The linked issue this PR says it addresses is one that looks to refactor the existing collector example, not add another one with a different exporter. I do not see the value in accepting these changes, it does not add a unique example of how to use this project. I would recommend it is closed.

The value of this is PR is to provide an example which keep the scope to just export OTLP to a collector running locally (via Docker).
This is also what was requested in the original issue.

Using this example does not require jaeger, prometheus and any other third-party backends, nor does it require users to use k8s to locally establish a cluster.

Using this example, you can clearly see the original trace data provided by open-telemetry, not through any other backend that developers might lose the original view of trace data. You can also use this example to understand the easiest way to generate trace data.

I keep the original example instead of refactoring it. Because the user can make enhanced observation of the trace data through other backends after using this example.

@dmathieu
Copy link
Member

I think naming this example logger is misleading. It may look like the example is about using the library to do logging.
I also agree with @MrAlias that we don't need a new example if one is too complicated. The existing example should be simplified, or refactored to be more understandable (for example with steps of increasing complexity).

@MrAlias
Copy link
Contributor

MrAlias commented Jun 24, 2022

I think naming this example logger is misleading. It may look like the example is about using the library to do logging. I also agree with @MrAlias that we don't need a new example if one is too complicated. The existing example should be simplified, or refactored to be more understandable (for example with steps of increasing complexity).

@lyl156 is this something you are willing to consider?

@MrAlias MrAlias added the response needed Waiting on user input before progress can be made label Jun 24, 2022
@lyl156
Copy link
Author

lyl156 commented Jun 25, 2022

What I can do now is to replace the original with mine, because I don't understand your exact thoughts on this issue.
If you can provide exact examples representing the result you want, I'd love to revise this pr.
If not, I will close this pr soon.

@lyl156 lyl156 closed this Jun 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
response needed Waiting on user input before progress can be made
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants