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 Azure collector #161

Merged
merged 13 commits into from
Aug 2, 2019
Merged

Conversation

pakrym
Copy link
Contributor

@pakrym pakrym commented Jul 24, 2019

No description provided.

@pakrym pakrym force-pushed the pakrym/azure-collector branch from f49ebe8 to 936e674 Compare July 26, 2019 21:47
@pakrym pakrym marked this pull request as ready for review July 26, 2019 22:08
@pakrym pakrym closed this Jul 29, 2019
@pakrym pakrym reopened this Jul 29, 2019
{ { "HttpHandlerDiagnosticListener", (t, s) => new HttpHandlerDiagnosticListener(t, s) } },
{
{ "HttpHandlerDiagnosticListener", (t, s) => new HttpHandlerDiagnosticListener(t, s) },
{ "Azure.Clients", (t, s) => new AzureSdkDiagnosticListener("Azure.Clients", t, sampler) },
Copy link

@lmolkova lmolkova Jul 30, 2019

Choose a reason for hiding this comment

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

s is not necessarily the same as sampler - sampler could be null and this crazy lambda underneath falls back to something. So please use s

Copy link
Contributor Author

Choose a reason for hiding this comment

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

s is lambda that takes HttpRequestMessage, what do I pass into it?

Choose a reason for hiding this comment

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

no I mean this lambda:

x =>
                {
                    ISampler s = null;
                    try
                    {
                        s = options.CustomSampler(x);
                    }
                    catch (Exception e)
                    {
                        s = null;
                        DependenciesCollectorEventSource.Log.ExceptionInCustomSampler(e);
                    }
                    return s ?? sampler;
                    });

I.e. "Azure.Clients", (t, s) => new AzureSdkDiagnosticListener("Azure.Clients", t, sampler) -> "Azure.Clients", (t, s) => new AzureSdkDiagnosticListener("Azure.Clients", t, s)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, type of s is Func<HttpRequestMessage, ISampler> how do I use it in my listener to get a sampler? Always pass null in?

Choose a reason for hiding this comment

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

oh, I see. sorry. let me think for a moment

Choose a reason for hiding this comment

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

how about this? #170


var span = this.tracer.SpanBuilder(operationName)
.SetCreateChild(false)
.SetSpanKind(SpanKind.Client)

Choose a reason for hiding this comment

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

oh, by the way, this is not always right

  • http spans should have kind = client
  • convenience layer spans should have kind = internal

This is important for Azure Monitor and maybe other backends for better user experience.

@lmolkova lmolkova merged commit 5d30391 into open-telemetry:master Aug 2, 2019
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.

3 participants