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

Don't trace anything if the backend is not available. #4952

Closed
wants to merge 1 commit into from
Closed

Don't trace anything if the backend is not available. #4952

wants to merge 1 commit into from

Conversation

zifeihan
Copy link
Member

Please answer these questions before submitting pull request

  • Why submit this pull request?

  • Bug fix

  • New feature provided

  • Improve performance

  • Related issues


Bug fix

  • Bug description.

  • How to fix?


New feature or improvement

  • Don't trace anything if the backend is not available.

@codecov
Copy link

codecov bot commented Jun 20, 2020

Codecov Report

Merging #4952 into master will decrease coverage by 19.40%.
The diff coverage is n/a.

Impacted file tree graph

@@              Coverage Diff              @@
##             master    #4952       +/-   ##
=============================================
- Coverage     50.24%   30.83%   -19.41%     
+ Complexity     2752     1496     -1256     
=============================================
  Files           763      613      -150     
  Lines         18871    15308     -3563     
  Branches       1851     1437      -414     
=============================================
- Hits           9481     4720     -4761     
- Misses         8632    10238     +1606     
+ Partials        758      350      -408     
Impacted Files Coverage Δ Complexity Δ
...he/skywalking/oap/server/core/source/GCPhrase.java 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (-1.00%)
...walking/oap/server/core/source/MemoryPoolType.java 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (-1.00%)
...alking/oap/server/core/analysis/record/Record.java 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (-1.00%)
...ng/oap/server/core/analysis/config/NoneStream.java 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (-1.00%)
...g/oap/server/core/analysis/metrics/SumMetrics.java 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (-4.00%)
...server/core/analysis/metrics/DoubleAvgMetrics.java 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (-4.00%)
...ement/ui/template/UITemplateManagementService.java 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (-6.00%)
...ng/oap/server/storage/plugin/jdbc/SQLExecutor.java 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (-4.00%)
...ient/elasticsearch/ElasticSearchUpdateRequest.java 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (-2.00%)
...server/storage/plugin/jdbc/h2/dao/H2RecordDAO.java 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (-2.00%)
... and 313 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 520cee3...80c0371. Read the comment docs.

@wu-sheng
Copy link
Member

Do you check the code style? Please check the CI logs.

@wu-sheng wu-sheng added agent Language agent related. bug Something isn't working and you are sure it's a bug! labels Jun 20, 2020
@zifeihan
Copy link
Member Author

Do you check the code style? Please check the CI logs.

so sorry, not code style, some test will force check segment size when OAP backend not start. i will fix it.

@wu-sheng
Copy link
Member

I don't remember there is segment size test.

@zifeihan
Copy link
Member Author

zifeihan commented Jun 21, 2020

I don't remember there is segment size test.

like this,
org.apache.skywalking.apm.agent.core.remote.TraceSegmentServiceClientTest#testSendTraceSegmentWithoutException

    public void testSendTraceSegmentWithoutException() throws InvalidProtocolBufferException {
        grpcServerRule.getServiceRegistry().addService(serviceImplBase);

        AbstractSpan firstEntrySpan = ContextManager.createEntrySpan("/testFirstEntry", null);
        firstEntrySpan.setComponent(ComponentsDefine.TOMCAT);
        Tags.HTTP.METHOD.set(firstEntrySpan, "GET");
        Tags.URL.set(firstEntrySpan, "127.0.0.1:8080");
        SpanLayer.asHttp(firstEntrySpan);
        ContextManager.stopSpan();

        serviceClient.consume(storage.getTraceSegments());

        assertThat(upstreamSegments.size(), is(1));
        SegmentObject traceSegmentObject = upstreamSegments.get(0);
        assertThat(traceSegmentObject.getSpans(0).getRefsCount(), is(0));
        assertThat(traceSegmentObject.getSpansCount(), is(1));

        SpanObject spanObject = traceSegmentObject.getSpans(0);
        assertThat(spanObject.getSpanType(), is(SpanType.Entry));
        assertThat(spanObject.getSpanId(), is(0));
        assertThat(spanObject.getParentSpanId(), is(-1));
    }

@wu-sheng
Copy link
Member

Keep trace should be OFF, I think. Is there a case open this?

@zifeihan
Copy link
Member Author

@wu-sheng sorry, origin code is ok, this Config.Agent.KEEP_TRACING means Keep tracing even the backend is not available... sorry.

@zifeihan zifeihan closed this Jun 22, 2020
@wu-sheng wu-sheng added wontfix This will not be worked on and removed bug Something isn't working and you are sure it's a bug! labels Jun 22, 2020
@wu-sheng wu-sheng added this to the 8.1.0 milestone Jun 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
agent Language agent related. wontfix This will not be worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants