-
Notifications
You must be signed in to change notification settings - Fork 9
Adds support for Microsoft Azure Application Insights(AppInsights) as a datastore for Zipkin #27
Conversation
updating recent commit in openzipkin:zipkin-azure
7890521
to
74e297f
Compare
7c36d14
to
90f2f24
Compare
90f2f24
to
e304bcd
Compare
e304bcd
to
e4401f2
Compare
zipkin: | ||
storage: | ||
applicationinsights: | ||
instrumentationKey: ${AI_INSTRUMENTATION_KEY:} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if it's a section applicationinsights
- why we need to prefix it with AI_
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is readable here in config file.. but during actual configuration user jus sets AI_INSTRU... and it would be much clearer to have prefix.
zipkin: | ||
storage: | ||
applicationinsights: | ||
instrumentationKey: ${AI_INSTRUMENTATION_KEY:} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
environment variable for instrumentation key that we typically use is APPINSIGHTS_INSTRUMENTATIONKEY
https://github.com/Microsoft/ApplicationInsights-dotnet/blob/151509eac5e23fcbd09f3a8989a35d1099703957/src/Core/Managed/Shared/Extensibility/Implementation/TelemetryConfigurationFactory.cs#L22
It is not ideal as it uses a wrong name appinishgts
, but it is what we use everywhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will change it to APPINSIGHTS.. If thats norm.... i just thought AI_IN... will make the key shorter..and in power shell I often type so less work :)
applicationinsights: | ||
instrumentationKey: ${AI_INSTRUMENTATION_KEY:} | ||
applicationId: ${AI_APPLICATION_ID:} | ||
apiKey: ${AI_API_KEY:} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how fast are you running to the query limit? Using Azure Active Directory authentication may eliminate this problem, but may be hard to implement. Just a thought in case you have an idea how to fix it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ran few thousands and didn't face much issues. For higher loads .. I am thinking of rotating API_Keys as a solution ..but would like to know about AAD.
} | ||
String res = gson.toJson(jsonElement); | ||
String msg = "{ \"Span\":" + res + "}"; | ||
telemetry.trackTrace(msg, SeverityLevel.Critical, spanProps); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see you are using Application Insights like a storage. I'd advice to use Application Insights data model instead Use requests and dependencies for incoming and outgoing spans. Like explained here: https://docs.microsoft.com/azure/application-insights/application-insights-correlation#open-tracing-and-application-insights
<!-- HTTP request component (not required for bare API) --> | ||
|
||
<TelemetryModules> | ||
<Add |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since you only track telemetry manually - you do not need any auto-collection modules and initializers.
storage/applicationinsights/pom.xml
Outdated
</dependency> | ||
<dependency> | ||
<groupId>com.microsoft.azure</groupId> | ||
<artifactId>applicationinsights-web</artifactId> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you monitor zipkin itself or just use it to send telemetry? I'd advice to use applicationinsights
if you do not use any auto-collection modules
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok.. I am not monitoring zipkin. So, I will change this. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By "applicaitoisisghts", Do you mean "applicationinsights-core"?
Merge from master to pick up latest changes
@@ -33,6 +35,7 @@ | |||
|
|||
<modules> | |||
<module>collector-eventhub</module> | |||
<module>storage-applicationinsights</module> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the difference between collector and storage? Should we have Application Insights collector AND storage. So collection part can be used separately from the storage?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Collector is the transport protocol(default Http) you listen on ... for receiving spans. Right now we are using Http - so, I guess we might not have a separate AI collector unless you have a better business case where AI collector helps.
You can directly hit your storage with Spans if that is your question. It is fine as long as Zipkin REST API gets the right data while reading.
Collectors like Eventhub could have many clients other than storage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can collector be used in-process? So we do not need a sidecar container hosting it? Also it looks to me this is a regular setup. Like here:
Production configuration
- Numerous collectors zipkin collectors per data centre – no UI enabled
- One zipkin UI server per data centre – firewalled and hidden behind an apache web server to provide authentication and auditing. Not accessible for span collector service
Hi there. Any update around this PR approval? I'm trying to create a docker images (without luck so far) and this PR not approved yet makes things a little more difficult. |
@marcote, thanks for interest. @praveenbarli and I working on some refinements on how spans should be stored. This way we will be able to see spans natively in Application Insights and see Application Insights's collected data in Zipkin. We've estimated another week to complete this work |
@marcote Nice to see you trying out this PR. We are working with admins @adriancole @aliostad to get this checked in. However I would like you to continue with your setup and let I and @SergeyKanzhelev know if you face any challenges. That way we can work in parallel and also a succesfull installation and testing in your system would be a positive push for admins to review/check in changes. |
…com/praveenbarli/zipkin-azure into createautoconfigureappinsightmodule
…zipkin-azure into writetoaidatamodel
data model changes to add name, id and dependency for sr annotaiton
add zipkin span in json as custom property to requests/dependencies t…
as discussed with @dhaval24 and others at dinner last week. The way to move things forward is to change this into a "write only" component by making it a combination of translator (zipkin2 -> azure span) a sender (for zipkin-reporter meaning apps can embed it), and possibly a write-only storage component which can allow a zipkin server to proxy spans to your service. Possibly you'd also want a brave component if you are using trace context format with some azure specific trace state. key things that are still the case are that we'd want some end user involved to help justify the community efforts which result in new code. For example, even the existing code I have done dozens of releases and that's something we shouldn't do unless end users are benefiting. For reference, here's the zipkin-gcp repo which has functionality you'd want https://github.com/openzipkin/zipkin-gcp |
@adriancole thanks a lot for bringing this back! Though, I still do not fully get this work, here is what my understanding is:
A question is:
Apologies if this is a very rudimentary question, but I am very new to Sleuth / Zipkins / Brave world :) @nikmd23 can comment on the justification, we did had many customers who were using Application Insights for Java with SpringBoot applications and were interested in getting slueth spans reported to AzureMonitor and be able to view the E2E traces on Azure UX. |
If this is write-only then it wouldn't allow use of zipkin UI, rather only
your UI. However, it is better to tease functionality apart like this as
otherwise it is endless. For example, not only did this stall out, but also
another attempt like influx. Rather than going for gold, probably best to
start with writing, then see if there is value moving forward in readback
etc.
…On Wed, Oct 3, 2018 at 12:54 AM Dhaval Doshi ***@***.***> wrote:
@adriancole <https://github.com/adriancole> thanks a lot for bringing
this back! Though, I still do not fully get this work, here is what my
understanding is:
1. This would enable the ZipKin view for people using AzureMonitor on
Zipkin UX. Am I correct ?
A question is:
1. Would the reverse be true? Will the spans emitted from sleuth able
to be viewed on AzureMonitor UX for tracing ?
Apologies if this is a very rudimentary question, but I am very new to
Sleuth / Zipkins / Brave world :) @nikmd23 <https://github.com/nikmd23>
can comment on the justification, we did had many customers who were using
Application Insights for Java with SpringBoot applications and were
interested in getting slueth spans reported to AzureMonitor and be able to
view the E2E traces on Azure UX.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#27 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAD61_vp-oFzidth0UWrHoj0Q_4Zu5fiks5ug5pMgaJpZM4NWXbJ>
.
|
closing because.. well it has been well over a year |
This PR contains changes for supporting Microsoft Azure Application Insights as a data store for Zipkin.