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

Add Quartz.OpenTracing support #1006

Merged
merged 4 commits into from Oct 25, 2020
Merged

Conversation

PavelStefanov
Copy link
Contributor

Hi everyone!

I was really happy about OpenTelemetry support but it's still in beta and most projects use OpenTracing instead.
I added OpenTracing support and it'd be great if you could help me to finish it correctly and publish this NuGet package.

Copy link
Member

@lahma lahma left a comment

Choose a reason for hiding this comment

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

Looks like a great addition. I don't have much to give feature wise as I'm unfamiliar with OpenTracing. It seems that the PR is correctly hooked to produce the NuGet package already.

There's also adding documentation to docs folder for a new integration package and updating the changelog, but that can be left for later too.

<RootNamespace>Quartz.OpenTracing</RootNamespace>
<Title>Quartz.NET OpenTracing integration</Title>
<Description>Quartz.NET OpenTracing integration</Description>
<GenerateDocumentationFile>True</GenerateDocumentationFile>
Copy link
Member

Choose a reason for hiding this comment

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

You can add <Authors>YourNameHere, Quartz.NET</Authors>

Copy link
Member

Choose a reason for hiding this comment

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

And the indents seem to be a bit off I think (too much)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!
There's awesome approach to maintain coding style it's EditorConfig. It's supported by VS, Rider and VS Code. I generated .editorconfig using IntelliCode. It analyses current project and creates relevant rules. If you don't mind. I hope this helps in the future.

But I've few questions e.g. Quartz.OpenTelemetry.Instrumentation.csproj has some special vars like $(TargetFullFrameworkVersion) and $(Description), should I add the same things?


namespace Quartz.OpenTracing
{
public class QuartzDiagnosticOptions
Copy link
Member

Choose a reason for hiding this comment

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

I think OpenTelemetry share the same concepts here. Should these options be unified to be quite the same between the integrations, thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it looks similar and indeed OpenTracing and OpenTelemetry have the same concept, but there're difference between implementations.

  • Looks like in OpenTelemetry implementation we try to subscribe to Veto event also but I think for tracing we interest only for Execute event cuz we need tracing some long operations not just logging some statements
  • In OpenTracing impl we have only event name and payload from diagnostic and then we manually create span and fill all tags ourself therefore we've ComponentName property and OperationNameResolver func in the options and users can override these as they want. At the same time as I can see OpenTelemetry gets already filled Activity and can only add some tags, it can't change OperationName cuz it's already filled from DiagnosticListener.
  • If we merge these options we'll must move it to Quartz project or will create some new project to share options. I don't think so it's good idea to move it to Quartz, it's nothing connecting to main project it's implementation details of tracing.
    And then we have to support it between impls and I expect that after OpenTelemetry is released it'll be main approach for distributing tracing and it can develop faster.
    P.S. May be we should change OpenTelemetry like OpenTracing cuz Activity's name it's "Quartz.Job.Execute" now but in tracing we want to see JobName also, but Activity should have general name for subscription.

But I'm not familiar with OpenTelemetry implementation so well and mb I don't know something.

@lahma lahma added this to the 3.3 milestone Oct 23, 2020
@PavelStefanov
Copy link
Contributor Author

I also can add example code in Quartz.Examples.AspNetCore project but it looks like so overloaded already.

And I suggest to create another issue for docs.

@lahma lahma modified the milestones: 3.3, 3.2.3 Oct 25, 2020
@lahma lahma merged commit 0bb14fc into quartznet:master Oct 25, 2020
@lahma
Copy link
Member

lahma commented Oct 25, 2020

Thank you for this!

I'll add some documentation and update both tracing solutions to include a note that they use beta packages and API might still change a bit. All PRs to fine-tune are greatly welcome as you are the one who knows how this thing actually is expected work 🙂

@lahma
Copy link
Member

lahma commented Oct 25, 2020

I've added some basic documentation with 3ce179f and will already go live and state that the unpublished version will be required.

@lahma
Copy link
Member

lahma commented Oct 31, 2020

Version 3.2.3 of Quartz.NET has been released including this new feature, thanks again for the contribution!

@PavelStefanov PavelStefanov deleted the opentracing branch October 31, 2020 11:29
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.

None yet

2 participants