Add LoggingTracer sample #92
Conversation
discostu105
requested review from
austinlparker,
lmolkova and
SergeyKanzhelev
as code owners
|
@discostu105 can you please help me understand which problem does it solve? Is it available in other languages? Why does it have to live in this repo? |
samples/LoggingTracer/LoggingTracer.AspNetCore/LoggingTracer.AspNetCore.csproj
Outdated
Show resolved
Hide resolved
samples/LoggingTracer/LoggingTracer.AspNetCore/Pages/Error.cshtml
Outdated
Show resolved
Hide resolved
samples/LoggingTracer/LoggingTracer.AspNetCore/wwwroot/js/site.js
Outdated
Show resolved
Hide resolved
| public IScope StartScopedSpan() | ||
| { | ||
| Logger.Log($"SpanBuilder.StartScopedSpan"); | ||
| return new CurrentSpanUtils.LoggingScope(new LoggingSpan(spanName, spanKind)); |
There was a problem hiding this comment.
you don't need to create a new span every time. One instance should suffice
There was a problem hiding this comment.
ok. creating the span in .ctor now and re-use that instance.
interesting question though: If I use one SpanBuilder, should I be able to create multiple spans from that (multiple calls to StartScopedSpans)?
samples/LoggingTracer/readme.md
Outdated
|
|
||
| This is supposed to be a super-simple API implementation that helps to understand OpenTelemetry API layer. | ||
|
|
||
| It basically just logs every single API call, with thread-id and a time-offset since start. |
There was a problem hiding this comment.
perhaps getters should only log.
One of the principles of OpenTelemetry is to allow to create an alternative implementations. I think it's a good showcase how this can be done. At this stage it creates some liability as API is changing rapidly. But in general, it's OK to have. @discostu105 my only concern is amount of code you bring with the ASP.NET example. The worst thing is to get a security notification about the repo when some issue found in jquery or bootstrap. So if it possible - can you please minimize the amount of code you submit to simplify maintenance |
|
Thanks for explanation @SergeyKanzhelev. Assuming it lives in the different repo - what changes? Since it's an example there is nothing enforcing it to work - just to compile. If we want to keep it going forward in this repo - can we enforce some testing to make sure it still works after we change things? |
…ace-oss-contrib/opentelemetry-dotnet into add-loggingtracer-samples # Conflicts: # samples/Exporters/Exporters.csproj
For me it's was an exercise to:
I thought that could be useful for others as an example. Maybe to run it to see API calls on the console themself. The "liability" concerning the API changes is a valid concern. But you have the same with tests. The implementation is also very small and simple, so should be easy to adapt. If you think it's not worth putting it into the repo, I am also fine with that. I could put it into my own sample repo. |
…dhere to code style better (usings), don't log getters
|
|
@lmolkova thanks. adressed your comments. |
|
@discostu105 sorry async main suggestion broke the build. Could you please update Directory.Build.props in root as mentioned here? https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/configure-language-version#configure-multiple-projects , add |
|
@SergeyKanzhelev @austinlparker please merge! |
|
@discostu105 can you please fix the build? WIth the recent changes in master you need some stylecop fixes |
…try-dotnet into add-loggingtracer-samples
…try-dotnet into add-loggingtracer-samples
…ace-oss-contrib/opentelemetry-dotnet into add-loggingtracer-samples
|
Branch is up-to-date and build is fixed. |
|
Thanks! |
…elemetry#92) Missed a .Tests on the end of the dotnet test step.
This is supposed to be a super-simple API implementation that helps to understand OpenTelemetry API layer.
It basically just logs every single API call, with thread-id and a time-offset since start.
#89