[feat][broker]: intercept transaction begin and end events for observability #14613

madhavan-narayanan · 2022-03-09T04:07:41Z

(If this PR fixes a github issue, please add Fixes #<xyz>.)

Fixes #

(or if this PR is one task of a github issue, please add Master Issue: #<xyz> to link to the master issue.)

Master Issue: #12858

Motivation

This is to provide visibility to cluster operators when transactions are handled by the broker. Currently there is no way to know when a transaction begins and the produce/ack operations contained in it and the end status of the transaction (commit/rollback). Without this information, it is tough to support end customers where there are queries about missing messages or failed processing

Modifications

Enhanced org.apache.pulsar.broker.intercept.BrokerInterceptor interface to include additional events for tracing transaction events

Verifying this change

Make sure that the change passes the CI checks.

(Please pick either of the following options)

This change is a trivial rework / code cleanup without any test coverage.

(or)

This change is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

Added integration tests for end-to-end deployment with large payloads (10MB)
Extended integration test for recovery after broker failure

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

Dependencies (does it add or upgrade a dependency): ( no)
The public API: ( no)
The schema: (no)
The default values of configurations: (no)
The wire protocol: (no)
The rest endpoints: (no)
The admin cli options: (no)
Anything that affects deployment: (no)

Documentation

Check the box below or label this PR directly (if you have committer privilege).

Need to update docs?

doc-required

(If you need help on updating docs, create a doc issue)
no-need-doc

(Please explain why)
doc

(If this PR contains doc changes)

github-actions · 2022-03-09T04:19:47Z

@madhavan-narayanan:Thanks for your contribution. For this PR, do we need to update docs?
(The PR template contains info about doc, which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)

github-actions · 2022-03-09T05:09:30Z

@madhavan-narayanan:Thanks for providing doc info!

gaoran10 · 2022-03-09T09:45:14Z

Could we add these interceptor methods in PulsarCommandSender? And we could add a new test in BrokerInterceptorTest.

codelipenghui

Please help add tests for covering the new changes.

codelipenghui · 2022-03-15T02:09:33Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/intercept/BrokerInterceptor.java

+     * @param tcId Transaction Coordinator Id
+     * @param txnID Transaction ID
+     */
+    default void beginTxn(long tcId, String txnID) {


It's better to use newTxn to keep consistent with the command.

And from the implementation, I think it should be txnOpened and txnEnded?

codelipenghui · 2022-03-15T02:12:51Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java

                    ctx.writeAndFlush(Commands.newTxnResponse(requestId, txnID.getLeastSigBits(),
                            txnID.getMostSigBits()));
+                    if (getBrokerService().getInterceptor() != null) {
+                        getBrokerService().getInterceptor().beginTxn(command.getTcId(), txnID.toString());
+                    }


It's better to move to PulsarCommandSender so that we can have a unified management of sent commands and interception

Hi @codelipenghui ,
I have added unit tests and refactored code as you had recommended. Can you please review again?

gaoran10 · 2022-03-17T09:19:13Z

Currently, there is a method onPulsarCommand in the interface BrokerInterceptor, this method could handle all Pulsar commands. If we add specific methods txnOpened and txnEnded, I think it's confusing. Do we need to add events in BrokerInterceptor for all commands?

madhavan-narayanan · 2022-03-17T10:16:14Z

Currently, there is a method onPulsarCommand in the interface BrokerInterceptor, this method could handle all Pulsar commands. If we add specific methods txnOpened and txnEnded, I think it's confusing. Do we need to add events in BrokerInterceptor for all commands?

Hi @gaoran10 ,
Unfortunately the command objects available in the onPulsarCommand method do not have enough context to provide sufficient observability. For e.g the CommandEndTxnResponse object does not help to know if the transaction was committed or aborted.

gaoran10 · 2022-03-17T15:14:31Z

@madhavan-narayanan Maybe we could add a new param Object context for method onPulsarCommand to make it‘s suitable for more cases. I'm concerned that we need to add too many events in BrokerInterceptor.

madhavan-narayanan · 2022-03-18T01:53:29Z

@madhavan-narayanan Maybe we could add a new param Object context for method onPulsarCommand to make it‘s suitable for more cases. I'm concerned that we need to add too many events in BrokerInterceptor.

@gaoran10 , I understand your concern. I can add another overloaded method for 'onPulsarCommand' with additional 'context' parameter to avoid proliferation of callbacks in the future. But is the opaque 'Object context' any concern? Developers implementing the interceptor have to understand the structure and semantics of the context by browsing the broker code or from the documentation of 'onPulsarCommand' which I am afraid can become unwieldy and ambiguous later.

gaoran10 · 2022-03-18T07:23:05Z

@madhavan-narayanan Ok, the method onPulsarCommand could get all Pulsar commands, and some commands have their own event method in BrokerInterceptor, maybe it's more convenient for some users. We could improve the onPulsarCommand in the future if needed. Thanks

…ability (apache#14613) ### Motivation This is to provide visibility to cluster operators when transactions are handled by the broker. Currently there is no way to know when a transaction begins and the produce/ack operations contained in it and the end status of the transaction (commit/rollback). Without this information, it is tough to support end customers where there are queries about missing messages or failed processing

…ability (apache#14613) This is to provide visibility to cluster operators when transactions are handled by the broker. Currently there is no way to know when a transaction begins and the produce/ack operations contained in it and the end status of the transaction (commit/rollback). Without this information, it is tough to support end customers where there are queries about missing messages or failed processing

madhavan-narayanan and others added 4 commits January 11, 2022 15:10

Interceptors for transaction begin and end events

c7dff9f

Merge branch 'apache:master' into master

87c37ff

Merge branch 'apache:master' into master

683b766

Merge branch 'apache:master' into master

7371a70

github-actions bot assigned madhavan-narayanan Mar 9, 2022

github-actions bot added the doc-label-missing label Mar 9, 2022

github-actions bot added doc-not-needed Your PR changes do not impact docs and removed doc-label-missing labels Mar 9, 2022

gaoran10 requested review from 315157973, codelipenghui, congbobo184 and zymap March 9, 2022 09:46

codelipenghui reviewed Mar 15, 2022

View reviewed changes

codelipenghui added this to the 2.11.0 milestone Mar 15, 2022

madhavan-narayanan and others added 3 commits March 16, 2022 17:15

Merge branch 'apache:master' into master

c833b63

Merge branch 'apache:master' into master

9a8529e

pulsar-broker: Changes to address review comments in PR#14613

50243b7

codelipenghui approved these changes Mar 17, 2022

View reviewed changes

gaoran10 approved these changes Mar 18, 2022

View reviewed changes

codelipenghui changed the title ~~pulsar-broker: Intercept transaction begin and end events for observability~~ [feat][broker]: intercept transaction begin and end events for observability Mar 18, 2022

codelipenghui merged commit 86442ee into apache:master Mar 18, 2022

nicoloboschi mentioned this pull request Jan 5, 2023

[fix][txn] Always send correct transaction id in end txn response #19137

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[feat][broker]: intercept transaction begin and end events for observability #14613

[feat][broker]: intercept transaction begin and end events for observability #14613

madhavan-narayanan commented Mar 9, 2022 •

edited by codelipenghui

Loading

github-actions bot commented Mar 9, 2022

github-actions bot commented Mar 9, 2022

gaoran10 commented Mar 9, 2022 •

edited

Loading

codelipenghui left a comment

codelipenghui Mar 15, 2022

codelipenghui Mar 15, 2022

madhavan-narayanan Mar 17, 2022

codelipenghui Mar 15, 2022

madhavan-narayanan Mar 17, 2022

madhavan-narayanan Mar 17, 2022

gaoran10 commented Mar 17, 2022

madhavan-narayanan commented Mar 17, 2022

gaoran10 commented Mar 17, 2022

madhavan-narayanan commented Mar 18, 2022

gaoran10 commented Mar 18, 2022

[feat][broker]: intercept transaction begin and end events for observability #14613

[feat][broker]: intercept transaction begin and end events for observability #14613

Conversation

madhavan-narayanan commented Mar 9, 2022 • edited by codelipenghui Loading

Motivation

Modifications

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

github-actions bot commented Mar 9, 2022

github-actions bot commented Mar 9, 2022

gaoran10 commented Mar 9, 2022 • edited Loading

codelipenghui left a comment

Choose a reason for hiding this comment

codelipenghui Mar 15, 2022

Choose a reason for hiding this comment

codelipenghui Mar 15, 2022

Choose a reason for hiding this comment

madhavan-narayanan Mar 17, 2022

Choose a reason for hiding this comment

codelipenghui Mar 15, 2022

Choose a reason for hiding this comment

madhavan-narayanan Mar 17, 2022

Choose a reason for hiding this comment

madhavan-narayanan Mar 17, 2022

Choose a reason for hiding this comment

gaoran10 commented Mar 17, 2022

madhavan-narayanan commented Mar 17, 2022

gaoran10 commented Mar 17, 2022

madhavan-narayanan commented Mar 18, 2022

gaoran10 commented Mar 18, 2022

madhavan-narayanan commented Mar 9, 2022 •

edited by codelipenghui

Loading

gaoran10 commented Mar 9, 2022 •

edited

Loading