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

feature: add support for Pulsar #173

Merged
merged 5 commits into from
Mar 11, 2024
Merged

feature: add support for Pulsar #173

merged 5 commits into from
Mar 11, 2024

Conversation

CodePrometheus
Copy link
Contributor

summary

Pulsar is a native go client maintained by the apache pulsar team.

  • Update the docs.
  • Pass CI test.
  • Pass Plugin test.

test report

image

@CodePrometheus
Copy link
Contributor Author

Ref apache/skywalking#11937.

Maybe I need some time, please hold for a while, thanks.

AFAIK, We have Pulsar monitoring and agent plugins (Java is ready and Go are currently in progress) for Pulsar, skywalking-showcase needs to be added accordingly? I am willing to try.

@wu-sheng
Copy link
Member

Showcase is fine, it can't hold all plugin examples there.

@wu-sheng
Copy link
Member

Meanwhile, you should try and test service hierarchy for Pulsar, if you want. especially from virtual MQ to MQ server.

@wu-sheng wu-sheng added enhancement New feature or request plugin labels Feb 26, 2024
@CodePrometheus
Copy link
Contributor Author

Meanwhile, you should try and test service hierarchy for Pulsar, if you want. especially from virtual MQ to MQ server.

Copy that, I want to try it.

@wu-sheng wu-sheng added this to the 0.5.0 milestone Feb 27, 2024
@CodePrometheus
Copy link
Contributor Author

PTAL.

plugins/pulsar/pulsar/receive_consumer.go Show resolved Hide resolved
segments:
- segmentId: not null
spans:
- operationName: GET:/health
Copy link
Contributor

Choose a reason for hiding this comment

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

Please delete the GET:/health checker.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor Author

@CodePrometheus CodePrometheus Mar 9, 2024

Choose a reason for hiding this comment

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

I noticed that I have also retained this spans in other plugin's file, I'll delete it in future PR.

- { key: mq.topic, value: not null }
- { key: mq.msg.id, value: not null }
- operationName: Pulsar/sw-topic-1/Consumer
parentSpanId: 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the consumer parent span id is 0(span GET:/execute), I think It should be Pulsar/sw-topic-1/Producer. Am I right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the current test code, both consumer and producer are triggered by http.

- { key: status_code, value: '200' }
- segmentId: not null
spans:
- operationName: Pulsar/sw-topic-2/Producer/Callback
Copy link
Contributor

Choose a reason for hiding this comment

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

When will we have the Callback span, It's a local span and not reference? Could you please add the screenshot about this plugin test? I'm not sure what's the real show like.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now as shown in the figure, when the two test methods are executed separately, it's as expected.
image

Copy link
Contributor

Choose a reason for hiding this comment

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

Then, why does the async producer have the children consumer, but the synchronized producer doesn't?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's a problem with the test code, and I think I've found the solution, please take a look again, thx.

@CodePrometheus CodePrometheus changed the title [feature-WIP] add support for Pulsar feature: add support for Pulsar Mar 9, 2024
@CodePrometheus
Copy link
Contributor Author

go: /skywalking-go/go.work:3: unknown directive: toolchain
Very strange, I tried it locally and it was passed.
image

@mrproliu
Copy link
Contributor

mrproliu commented Mar 9, 2024

Could you take a look on the go.work file under the root directory? Maybe change this file in a mistake?

@wu-sheng wu-sheng requested a review from mrproliu March 11, 2024 02:30
@wu-sheng wu-sheng merged commit 37820ce into apache:main Mar 11, 2024
37 checks passed
@CodePrometheus CodePrometheus deleted the feat_pulsar branch March 11, 2024 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants