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

data model changes to add name, id and dependency for sr annotaiton #5

Merged
merged 6 commits into from
Jun 29, 2017

Conversation

praveenbarli
Copy link
Owner

@praveenbarli praveenbarli commented Jun 26, 2017

@SergeyKanzhelev please verify the changes.

@@ -68,6 +77,16 @@ public void accept(List<Span> spans) {
}
String res = gson.toJson(jsonElement);
String msg = "{ \"Span\":" + res + "}";
//data model changes
telemetry.getContext().getOperation().setId(traceId);
telemetry.getContext().getOperation().setName(span.name);
Copy link
Collaborator

Choose a reason for hiding this comment

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

span.name should got to dependency.name or request.name. Setting it on operation context is optional


for (Annotation annotation : span.annotations) {
if (annotation.value.equalsIgnoreCase(Constants.SERVER_RECV)) {
telemetry.trackDependency(Constants.SERVER_RECV, "request", new Duration(span.duration==null?0L:span.duration),
Copy link
Collaborator

Choose a reason for hiding this comment

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

SERVER_RECV means server received? When server receives - you need to track request. Dependency is when client send outgoing request.


String spanId = Long.toString(span.id);
String traceId = Long.toString(span.traceId);
String traceIdHigh = Long.toString(span.traceIdHigh);
Copy link
Collaborator

Choose a reason for hiding this comment

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

is there parent ID? Can you please place it into custom property for now.

Copy link
Collaborator

@SergeyKanzhelev SergeyKanzhelev left a comment

Choose a reason for hiding this comment

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

see comments

import com.microsoft.applicationinsights.telemetry.SeverityLevel;
import com.microsoft.applicationinsights.TelemetryConfiguration;
import com.microsoft.applicationinsights.telemetry.Duration;
<<<<<<< HEAD
Copy link
Collaborator

Choose a reason for hiding this comment

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

oh, something went wrong here

@praveenbarli
Copy link
Owner Author

Hi @SergeyKanzhelev, Added request on SS annotation and parentid to customprops. Please review.

for (Annotation annotation : span.annotations) {

if (annotation.value.equalsIgnoreCase(Constants.CLIENT_SEND)) {
String spanName = span.name !=null && !span.name.isEmpty()?span.name:Constants.CLIENT_SEND;
Copy link
Collaborator

Choose a reason for hiding this comment

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

does span in Zipkin has special annotation for an error? In Application Insights we rely on failure flag for many scenarios. So if we can deduct that span failed from annotations - it will improve those scenarios.

Copy link
Owner Author

@praveenbarli praveenbarli Jun 27, 2017

Choose a reason for hiding this comment

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

Yes, they suggest logging a binary annotation and have an open issue. We shall discuss further how we set AI flag.

@@ -68,6 +85,24 @@ public void accept(List<Span> spans) {
}
String res = gson.toJson(jsonElement);
String msg = "{ \"Span\":" + res + "}";
//data model changes
telemetry.getContext().getOperation().setId(traceId);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it should be a combination of two ids - traceIdHigh and traceId

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yes, Shall I concatenate both strings while setting id? How are we going to retrieve back?

Copy link
Collaborator

@SergeyKanzhelev SergeyKanzhelev left a comment

Choose a reason for hiding this comment

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

Looks OK. Please address comments

@praveenbarli praveenbarli merged commit ab7fc99 into createautoconfigureappinsightmodule Jun 29, 2017
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.

2 participants