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

Service name improvements #64

Merged
merged 4 commits into from
Jul 25, 2017
Merged

Service name improvements #64

merged 4 commits into from
Jul 25, 2017

Conversation

gpolaert
Copy link
Contributor

The aim to this PR is to improve how user set the service name for each contribution.
2 things to do:
1/ Set a default service name for common contribution
2/Add the ability for the users to override these defaults and keep the decorators logic hidden for them

@gpolaert gpolaert added the tag: do not merge Do not merge changes label Jul 24, 2017
@gpolaert gpolaert changed the title Service name improvments Service name improvements Jul 24, 2017
@@ -116,7 +116,6 @@ Modern web application frameworks such as Dropwizard or Spring Boot are automati
| Hibernate| 5.x | Please check the following [JDBC instrumentation](#jdbc-instrumentation) section |
| [MongoDB](https://github.com/opentracing-contrib/java-mongo-driver) | 3.x | Intercepts all the calls from the MongoDB client |
| [Cassandra](https://github.com/opentracing-contrib/java-cassandra-driver) | 3.2.x | Intercepts all the calls from the Cassandra client |
| [Elasticsearch](https://github.com/opentracing-contrib/java-elasticsearch-client) | 5.4.x | Intercepts all the calls from the ES Transport client |
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently not supported

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes let's work on it in another PR

@gpolaert
Copy link
Contributor Author

I moved the 2 yaml files from dd-java-agent to dd-trace. The decorators are dd-trace stuff and automatically added when the tracer is initialized by the resolver.

The dd-trace.yaml is more tricky. A part of the configuration is related to the dd-tracer (service name, writer, sampler) and another part is related to the java-agent (annotations, package ...)

- type: HTTPComponent
matchingValue: java-aws-sdk
setValue: aws-client
- type: URLAsResourceName
# - type: URLAsResourceName # Performance issues, no URL generalisation
Copy link
Contributor

Choose a reason for hiding this comment

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

yes we may think to remove that one, or provide a different implementation. It would be great if in someway we can have something like /api/:id rather than /api/42, /api/43 and so on.

@gpolaert
Copy link
Contributor Author

The way is looking now image

@gpolaert gpolaert removed the tag: do not merge Do not merge changes label Jul 25, 2017
Copy link
Contributor

@palazzem palazzem left a comment

Choose a reason for hiding this comment

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

@gpolaert just one comment about it, otherwise I think it's a great improvement!

@@ -116,7 +116,6 @@ Modern web application frameworks such as Dropwizard or Spring Boot are automati
| Hibernate| 5.x | Please check the following [JDBC instrumentation](#jdbc-instrumentation) section |
| [MongoDB](https://github.com/opentracing-contrib/java-mongo-driver) | 3.x | Intercepts all the calls from the MongoDB client |
| [Cassandra](https://github.com/opentracing-contrib/java-cassandra-driver) | 3.2.x | Intercepts all the calls from the Cassandra client |
| [Elasticsearch](https://github.com/opentracing-contrib/java-elasticsearch-client) | 5.4.x | Intercepts all the calls from the ES Transport client |
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes let's work on it in another PR

@@ -12,12 +12,12 @@

public DBComponent() {
super();
this.setMatchingTag(Tags.COMPONENT.getKey());
this.setMatchingTag(Tags.DB_TYPE.getKey());
Copy link
Contributor

Choose a reason for hiding this comment

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

Great! considering that this value is set when using OT JDBC module, I think it's a great default.

this.setSetTag(DDTags.SERVICE_NAME);
}

@Override
public boolean afterSetTag(DDSpanContext context, String tag, Object value) {
public boolean afterSetTag(final DDSpanContext context, final String tag, final Object value) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is a kind of optimization? why those arguments must be set as final?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, it's the save actions plugin rule. Final avoids some mistakes (but not all) related to object manipulations

Copy link
Contributor

Choose a reason for hiding this comment

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

Good to me so!

@@ -5,14 +5,8 @@ decorators:
matchingValue: java-okhttp
setValue: http-client
- type: DBComponent
matchingValue: java-mongo
Copy link
Contributor

Choose a reason for hiding this comment

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

By removing these values, it means that by default Tags.DB_TYPE is used even for Mongo right?

Copy link
Contributor Author

@gpolaert gpolaert Jul 25, 2017

Choose a reason for hiding this comment

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

I changed the behavior of this decorator.
Mongo, JDBC, Cassandra, ES decorate the DB_TYPE with the value we want.
So, I generalize the decorator.

Each time a DB_TYPE is set, the service name is set to the actual value.

This thing may change depending the way we provide a manual way to set the service name

Copy link
Contributor

Choose a reason for hiding this comment

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

Perfect!

- type: HTTPComponent
matchingValue: java-aws-sdk
setValue: aws-client
- type: URLAsResourceName
Copy link
Contributor

Choose a reason for hiding this comment

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

At the moment I agree to remove it, since we should design better that one.

Copy link
Contributor

@palazzem palazzem left a comment

Choose a reason for hiding this comment

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

LGTM!

this.setSetTag(DDTags.SERVICE_NAME);
}

@Override
public boolean afterSetTag(DDSpanContext context, String tag, Object value) {
public boolean afterSetTag(final DDSpanContext context, final String tag, final Object value) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Good to me so!

@@ -5,14 +5,8 @@ decorators:
matchingValue: java-okhttp
setValue: http-client
- type: DBComponent
matchingValue: java-mongo
Copy link
Contributor

Choose a reason for hiding this comment

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

Perfect!

@gpolaert
Copy link
Contributor Author

@palazzem I added some comments, let me know if I merge or not this PR

@gpolaert gpolaert merged commit 132c5c7 into master Jul 25, 2017
@gpolaert gpolaert deleted the gpolaert/contrib-decorators branch July 25, 2017 12:08
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