Skip to content

Conversation

@xuanyu66
Copy link
Contributor

@xuanyu66 xuanyu66 commented May 7, 2021

Fix

  • Add a unit test to verify that the fix works.
  • Explain briefly why the bug exists and how to fix it.

When thrift message has nested field, ServerInProtocolWrapper read Inner fieldStop and make context = null, then ServerInProtocolWrapper read outer fieldStop, which cause NPE.
So I move this logic to readMessageEnd() so that make sure it's invoked only once

ServerInProtocolWrapper#readFieldBegin()
    @Override
    public TField readFieldBegin() throws TException {
       ................
        if (field.type == TType.STOP) {
            Boolean haveCreatedSpan =
                    (Boolean) ContextManager.getRuntimeContext().get(HAVE_CREATED_SPAN);
            if (haveCreatedSpan != null && !haveCreatedSpan) {
                try {
                    AbstractSpan span = ContextManager.createEntrySpan(
                            context.getOperatorName(), createContextCarrier(null));
                    span.start(context.startTime);
                    span.tag(TAG_ARGS, context.getArguments());
                    span.setComponent(ComponentsDefine.THRIFT_SERVER);
                    SpanLayer.asRPCFramework(span);
                } catch (Throwable throwable) {
                    LOGGER.error("Failed to create EntrySpan.", throwable);
                } finally {
                    context = null;
                }

        return field;
    }
  • If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #.
  • Update the CHANGES log.

@wu-sheng wu-sheng requested a review from dmsolr May 7, 2021 03:47
@wu-sheng wu-sheng added this to the 8.6.0 milestone May 7, 2021
@wu-sheng wu-sheng added agent Language agent related. enhancement Enhancement on performance or codes plugin Plugin for agent or collector. Be used to extend the capabilities of default implementor. labels May 7, 2021
@codecov
Copy link

codecov bot commented May 7, 2021

Codecov Report

Merging #6909 (a5fcb54) into master (49d36cb) will decrease coverage by 14.38%.
The diff coverage is 5.71%.

❗ Current head a5fcb54 differs from pull request most recent head aa17b89. Consider uploading reports for the commit aa17b89 to get more accurate results
Impacted file tree graph

@@              Coverage Diff              @@
##             master    #6909       +/-   ##
=============================================
- Coverage     54.40%   40.01%   -14.39%     
+ Complexity     4227     2440     -1787     
=============================================
  Files          1844      824     -1020     
  Lines         39358    20674    -18684     
  Branches       4360     1976     -2384     
=============================================
- Hits          21411     8272    -13139     
+ Misses        16919    11740     -5179     
+ Partials       1028      662      -366     
Impacted Files Coverage Δ Complexity Δ
...er/receiver/envoy/EnvoyMetricReceiverProvider.java 73.33% <0.00%> (-14.67%) 8.00 <1.00> (ø)
...alking/oap/server/receiver/envoy/TCPOALDefine.java 0.00% <0.00%> (ø) 0.00 <0.00> (?)
...server/receiver/envoy/als/AbstractALSAnalyzer.java 0.00% <ø> (-50.00%) 0.00 <0.00> (ø)
...p/server/receiver/envoy/als/AccessLogAnalyzer.java 0.00% <0.00%> (ø) 0.00 <0.00> (?)
...er/receiver/envoy/als/LogEntry2MetricsAdapter.java 0.00% <ø> (-49.10%) 0.00 <0.00> (ø)
...ver/receiver/envoy/als/k8s/K8SServiceRegistry.java 0.00% <0.00%> (-0.81%) 0.00 <0.00> (ø)
...er/envoy/als/tcp/AbstractTCPAccessLogAnalyzer.java 0.00% <0.00%> (ø) 0.00 <0.00> (?)
...iver/envoy/als/tcp/TCPLogEntry2MetricsAdapter.java 0.00% <0.00%> (ø) 0.00 <0.00> (?)
...nvoy/als/tcp/k8s/K8sALSServiceMeshTCPAnalysis.java 0.00% <0.00%> (ø) 0.00 <0.00> (?)
...y/als/tcp/mx/MetaExchangeTCPAccessLogAnalyzer.java 0.00% <0.00%> (ø) 0.00 <0.00> (?)
... and 1490 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 004eef6...aa17b89. Read the comment docs.

Copy link
Member

@dmsolr dmsolr left a comment

Choose a reason for hiding this comment

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

LGTM

@wu-sheng wu-sheng merged commit 0036ea6 into apache:master May 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent Language agent related. enhancement Enhancement on performance or codes plugin Plugin for agent or collector. Be used to extend the capabilities of default implementor.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants