-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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
New feature provided : new plugin for influxdb-java client #4846
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4846 +/- ##
============================================
- Coverage 50.22% 49.97% -0.26%
+ Complexity 2751 2737 -14
============================================
Files 763 763
Lines 18872 18875 +3
Branches 1851 1851
============================================
- Hits 9478 9432 -46
- Misses 8635 8687 +52
+ Partials 759 756 -3 Continue to review full report at Codecov.
|
@dagmom please add the InfluxDB logo/icon to http://github.com/apache/skywalking-rocketbot-ui |
Thanks! The plugin-test is required. :) |
@dagmom Read this doc, https://github.com/apache/skywalking/blob/master/docs/en/guides/Plugin-test.md, and you could find many test cases in our repo, https://github.com/apache/skywalking/tree/master/test/plugin/scenarios |
ok , I've read the doc. |
support-version.list modify form old version and healthCheck use ping() api
Hi, I will be on vacation for next three days. Feel free to review this PR, and other PRs. Just be carefully, we are closing the 8.0 release. |
@dagmom rat check failed, please recheck. |
@dagmom there is TODO in the new codes, when it's ready, please ping us |
|
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.
Some review comments should be addressed before merging, and please rebase your branch on the latest master branch since your branch is out of date
<artifactId>apm-influxdb-2.x-plugin</artifactId> | ||
<description>This plugin is for use with InfluxDB 1.x.</description> |
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.
The artifactId
and the description
don't match, one says it's for 2.x
while the other says it's for 1.x
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.
apm-influxdb-2.x-plugin
means influxdb client lib version
This plugin is for use with InfluxDB 1.x.
means influxdb version , then i'll change to client version
* enhance InfluxDB InfluxDBFactory | ||
* really impl class {@link org.influxdb.impl.InfluxDBImpl} | ||
* | ||
* @since 2020/05/22 |
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.
Capitalize the first letter of the sentence please
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 does since date mean? Quiet unusual, from my understanding. We coulf know when be added through git log directly
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 does since date mean? Quiet unusual, from my understanding. We coulf know when be added through git log directly
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'll removesince
|
||
private static final String ENHANCE_CLASS = "org.influxdb.impl.InfluxDBImpl"; | ||
private static final String INTERCEPTOR_CLASS = "org.apache.skywalking.apm.plugin.influxdb.interceptor.InfluxDBConstructorInterceptor"; | ||
private static final String INFLUXDB_METHOD_INTERCEPT_CLASS = "org.apache.skywalking.apm.plugin.influxdb.interceptor.InfluxDBMethodInterceptor"; |
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.
private static final String INFLUXDB_METHOD_INTERCEPT_CLASS = "org.apache.skywalking.apm.plugin.influxdb.interceptor.InfluxDBMethodInterceptor"; | |
private static final String INFLUXDB_METHOD_INTERCEPT_CLASS = "org.apache.skywalking.apm.plugin.influxdb.interceptor.InfluxDBMethodInterceptor"; |
assertThat(SpanHelper.getComponentId(span), is(ComponentsDefine.INFLUXDB_JAVA.getId())); | ||
List<TagValuePair> tags = SpanHelper.getTags(span); | ||
assertThat(tags.get(0).getValue(), is("InfluxDB")); | ||
// assertThat(tags.get(1).getValue(), is("write ".concat(allArgument[0].toString()))); |
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.
Remove if useless, or fix it if assertion is failed
* InfluxDBExecutor | ||
* | ||
* @author guhao | ||
* @since 2020/6/3 |
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.
Remove author tag
/** | ||
* InfluxDBExecutorTest | ||
* | ||
* @author guhao | ||
* @since 2020/6/3 | ||
*/ | ||
public class InfluxDBExecutorTest { | ||
|
||
// @Test | ||
// public void testPing(){ | ||
// InfluxDBExecutor executor = new InfluxDBExecutor("http://localhost:8086"); | ||
// Pong pong = executor.ping(); | ||
// System.out.println(pong.getVersion()); | ||
// Assert.assertNotNull(pong.getVersion()); | ||
// } | ||
|
||
} |
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.
Remove this if it's useless
new InstanceMethodsInterceptPoint() { | ||
@Override | ||
public ElementMatcher<MethodDescription> getMethodsMatcher() { | ||
return InfluxDBMethodMatch.INSTANCE.getInfluxDBMethodMatcher(); |
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.
This is good for encapsulation, but it's a pitfall that we check third-party classes in classes whose name end with *Instrumentation
skywalking/apm-checkstyle/checkStyle.xml
Lines 122 to 129 in f3d907b
<module name="ImportControl"> | |
<property name="file" value="${import.control}"/> | |
<property name="path" value="apm-sniffer/(apm-sdk-plugin|bootstrap-plugins|optional-plugins)/.+/src/main/.+Instrumentation.java$"/> | |
</module> | |
<module name="ImportControl"> | |
<property name="file" value="${import.control}"/> | |
<property name="path" value="apm-sniffer/apm-toolkit-activation/.+/src/main/.+Activation.java$"/> |
to avoid issues like this #2871 , but this breaks the checks, although there is no third-party class in the InfluxDBMethodMatch
for now, other reviewers should pay attention to this class in the future, FYI @wu-sheng
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.
Question, why need this singleton. This method is not called in high frequently, and nothing cached there.
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 did it the other way
Another Quasar plugin got merged first, so, please change the component id, #4951. And update the codes. |
# Conflicts: # apm-protocol/apm-network/src/main/java/org/apache/skywalking/apm/network/trace/component/ComponentsDefine.java # oap-server/server-bootstrap/src/main/resources/component-libraries.yml # oap-server/server-core/src/test/resources/component-libraries.yml
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.
Agent setup document, Table of Agent Configuration Properties should be updated as you could collect parameters. And this feature should be tested plugin test tool, I haven't seen the parameters in the tags for now.
@wu-sheng As i known, influxdb-java is unlike jdbc api, it has no parameters. Just use |
If just influxql, why need a config item? |
@wu-sheng I looked at other plugins, like |
OK. Your test case and CI fails somehow, please fix them. |
…that in component-libraries.yml
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.
Merged your other PR, but this PR still doesn't allow me to update automatically(you closed that option). Please update the branch.
apm-sniffer/config/agent.config
Outdated
@@ -78,3 +78,6 @@ logging.level=${SW_LOGGING_LEVEL:INFO} | |||
|
|||
# mysql plugin configuration | |||
# plugin.mysql.trace_sql_parameters=${SW_MYSQL_TRACE_SQL_PARAMETERS:false} | |||
|
|||
# influxdb plugin configuration | |||
# plugin.influxdb.trace_influxql=${SW_INFLUXDB_TRACE_INFLUXQL:true} |
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.
This is not a widely used config, don't need to add it into the default agent.config. Putting it in the document should be enough.
@wu-sheng Where is this option? I can't find it, or invite to this fork repository |
I think at the PR page, there is a selector to provide upstream repo owner to edit your fork repo or branch. I can't see this at my side. |
Please answer these questions before submitting pull request
add new java sdk plugin : InfluxDB