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

OTLP log exporter #1964

Merged
merged 27 commits into from
Oct 7, 2021
Merged

Conversation

alanwest
Copy link
Member

@alanwest alanwest commented Apr 8, 2021

This is a first cut at an OTLP log exporter. It was relatively simple to do a proof-of-concept for this. Much of the code is nearly the same as the trace exporter. As such, there's definitely an opportunity to abstract out common functionality between traces and logs (likely benefitting metrics as well). I'll work on this.

Code duplication aside, I wanted to put this PR out there beginning a discussion of how we might ship this functionality. Currently, I have the implementation in the OpenTelemetry.Exporter.OpenTelemetryProtocol project. However, this cannot ship as a regular release until logs are deemed stable.

There's a couple options:

  • Leave the code in this project and ship a prerelease version of the package.
  • Move the code to a separate project - e.g., OpenTelemetry.Exporter.OpenTelemetryProtocol.Logs - and ship this as a prerelease.

@codecov
Copy link

codecov bot commented Apr 8, 2021

Codecov Report

Merging #1964 (5f169b5) into main (a2daad1) will decrease coverage by 0.82%.
The diff coverage is 2.32%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1964      +/-   ##
==========================================
- Coverage   80.11%   79.29%   -0.83%     
==========================================
  Files         251      254       +3     
  Lines        8289     8375      +86     
==========================================
  Hits         6641     6641              
- Misses       1648     1734      +86     
Impacted Files Coverage Δ
...ementation/ExportClient/OtlpGrpcLogExportClient.cs 0.00% <0.00%> (ø)
...etryProtocol/Implementation/LogRecordExtensions.cs 0.00% <0.00%> (ø)
....Exporter.OpenTelemetryProtocol/OtlpLogExporter.cs 0.00% <0.00%> (ø)
...OpenTelemetry/Metrics/BaseExportingMetricReader.cs 89.28% <ø> (ø)
...Telemetry/Metrics/PeriodicExportingMetricReader.cs 0.00% <0.00%> (ø)
...tation/OpenTelemetryProtocolExporterEventSource.cs 61.29% <100.00%> (+2.66%) ⬆️
...Zipkin/Implementation/ZipkinExporterEventSource.cs 63.63% <0.00%> (-9.10%) ⬇️
...emetry.Api/Internal/OpenTelemetryApiEventSource.cs 73.52% <0.00%> (-5.89%) ⬇️
...nTelemetry/Internal/OpenTelemetrySdkEventSource.cs 74.28% <0.00%> (+0.95%) ⬆️

@gillg
Copy link

gillg commented May 26, 2021

Any news about that ?
About remaining Todo have you some ideas ? On our side we made a custom implementation and we faced with some issues related to Microsoft logger because it only support 5 log level constants which is not really compatible with OTEL.

Does anyone know if this lib should be changed il the future ? Else we just need a static mapping in this PR.

@edisonpaul4
Copy link

Hi guys! Great job! any update on this?

@jgarces21
Copy link

Hi all!
This is a great first step towards implementing the Log exporter on .Net
Can @cijothomas take a look at this PR?

Cheers!

@alanwest
Copy link
Member Author

alanwest commented Jun 3, 2021

It may not take too much to get this into a usable state. However, currently folks are primarily focused on supporting metrics, so it may be difficult get an eye on further logs support in the near term.

@jubenavides
Copy link

Hi, any update about this?

@ia-cefernan
Copy link

Hi everyone!, any updates on this?

@ia-jccontre
Copy link

It may not take too much to get this into a usable state. However, currently folks are primarily focused on supporting metrics, so it may be difficult get an eye on further logs support in the near term.

Is there any update on the log exporter?

@rodroxrom
Copy link

Hi all, can help us with this improvement?
Thanks!

@alanwest alanwest changed the base branch from main to otlp-logs September 24, 2021 23:10
@alanwest alanwest marked this pull request as ready for review September 24, 2021 23:11
@alanwest alanwest requested a review from a team September 24, 2021 23:11
@alanwest alanwest changed the title [WIP] OTLP log exporter OTLP log exporter Sep 24, 2021
@alanwest
Copy link
Member Author

I've retargeted this PR to a feature branch and set it ready to review. We could consider doing a pre-release version with the this functionality.

Another option we discussed a couple months back in the SIG meeting was the idea of hosting this exporter in the OpenTelemetry.Contrib.Preview package - but I liked this idea less after giving it a try. It required copying too many internal dependencies from the OpenTelemetry.Exporter.OpenTelemetryProtocol project to make work.

Also, when #2316 lands, it will require a little refactor to the log exporter and also some work to make it work with http/protobuf transport. If we land this to a feature branch these things could be a follow up.

@cijothomas
Copy link
Member

We discussed this in todays SIG, and agreed to make this a separate package (pre-release tag), agreeing to the original idea below:

Move the code to a separate project - e.g., OpenTelemetry.Exporter.OpenTelemetryProtocol.Logs - and ship this as a prerelease.

@reyang
Copy link
Member

reyang commented Sep 29, 2021

We discussed this in todays SIG, and agreed to make this a separate package (pre-release tag), agreeing to the original idea below:

Move the code to a separate project - e.g., OpenTelemetry.Exporter.OpenTelemetryProtocol.Logs - and ship this as a prerelease.

Maybe a quick & small topic that I want to get some ideas: are we thinking about having a single TCP connection that we can export logs/traces/metrics to OpenTelemetry Collector?

Put the question in a different way - if I use logs/metrics/traces and I don't want to have 3 OTLP exporters each managing their own connection / authz, what do I do?

@alanwest
Copy link
Member Author

Put the question in a different way - if I use logs/metrics/traces and I don't want to have 3 OTLP exporters each managing their own connection / authz, what do I do?

Good question, I've had this thought in the back of my mind as well. Opened an issue (#2426) and shared some initial brainstorming.

@alanwest alanwest changed the base branch from otlp-logs to main September 30, 2021 23:57
// limitations under the License.
// </copyright>

#if NET461 || NETSTANDARD2_0 || NETSTANDARD2_1
Copy link
Member

Choose a reason for hiding this comment

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

do we need this conditional at all?

Copy link
Member

@cijothomas cijothomas left a comment

Choose a reason for hiding this comment

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

LGTM. Left a comment about the need for conditional targetting.

@cijothomas cijothomas merged commit 1074311 into open-telemetry:main Oct 7, 2021
@alanwest alanwest deleted the alanwest/otlp-logs branch October 7, 2021 20:07
rkbodenner added a commit to rkbodenner/opentelemetry-dotnet that referenced this pull request Oct 14, 2021
alanwest added a commit that referenced this pull request Oct 14, 2021
* Mention logs as an option for OTLP exporter

Logs exporter recently landed here: #1964

* Mention logs support, requiring separate package

* Relative link within repo

* Create README.md

* Fix relative path and line length

* Fix relative path

* NuGet badges

Co-authored-by: Alan West <[email protected]>

* Use a file name to fix path warning

* Prereq is implicit already w/NuGet dependency

Co-authored-by: Cijo Thomas <[email protected]>
Co-authored-by: Alan West <[email protected]>
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.