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

feat: Implement initial pulsar support #1405

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

jaysonsantos
Copy link

@jaysonsantos jaysonsantos commented Oct 25, 2022

Description

This adds an initial support for pulsar

Type of change

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

Initial integration tests were run with the following:

import threading

import pulsar
from pulsar import ConsumerType, InitialPosition


def main():
    client = pulsar.Client("pulsar://localhost:6650")
    print(client)
    producer = client.create_producer("sample")
    
    producer.send(b"hello world")

    consumer = client.subscribe("sample", "consumer-1", consumer_type=ConsumerType.KeyShared, initial_position=InitialPosition.Earliest)
    print(consumer.receive(1_000))

    done = threading.Event()

    def consume(consumer, message):
        print('Consumer', consumer, 'Message', message)
        done.set()
    consumer2 = client.subscribe("sample", "consumer-2", message_listener=consume, consumer_type=ConsumerType.KeyShared, initial_position=InitialPosition.Earliest)
    consumer2.resume_message_listener()

    done.wait()
    consumer2.close()
    consumer.close()

    producer.close()
    client.close()

Does This PR Require a Core Repo Change?

  • Yes. - Link to PR:
  • No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

@jaysonsantos jaysonsantos force-pushed the initial-pulsar-support branch 2 times, most recently from 3d3b1f3 to d63243c Compare October 25, 2022 21:25
@jaysonsantos jaysonsantos marked this pull request as ready for review October 26, 2022 10:35
@jaysonsantos jaysonsantos requested a review from a team October 26, 2022 10:35
)

span.set_attribute(
SpanAttributes.MESSAGING_KAFKA_PARTITION, message.partition()

Choose a reason for hiding this comment

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

Suggested change
SpanAttributes.MESSAGING_KAFKA_PARTITION, message.partition()
SpanAttributes.MESSAGING_PULSAR_PARTITION, message.partition()

Copy link
Contributor

@ocelotl ocelotl left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! Please take a look at the comments.

@jaysonsantos
Copy link
Author

I just realized that when I ran the linter locally, it got skipped 🤦
image
I will send the fixes soon and run those targets that failed

@jaysonsantos jaysonsantos force-pushed the initial-pulsar-support branch from 2bdb46a to 1eb2a31 Compare November 18, 2022 13:43
@jaysonsantos
Copy link
Author

Done, I just rebased it with main

@jaysonsantos jaysonsantos requested a review from ocelotl November 18, 2022 13:43
tox.ini Outdated Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
Copy link
Contributor

@ocelotl ocelotl left a comment

Choose a reason for hiding this comment

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

Some minor comments only

@jaysonsantos jaysonsantos requested a review from ocelotl November 30, 2022 18:48
@jaysonsantos
Copy link
Author

Hi @ocelotl i sent the fixes again, the broken tests seem unrelated to my changes, they break on http* libray

@jaysonsantos
Copy link
Author

@ocelotl I notice a bug in send_sync and fixed and a few tests mocking the underlying c library that pulsar uses.

@jaysonsantos jaysonsantos force-pushed the initial-pulsar-support branch from 63f37e8 to d451397 Compare December 9, 2022 15:33
jaysonsantos and others added 9 commits March 21, 2023 16:54
Signed-off-by: Jayson Reis <santosdosreis@gmail.com>
…elemetry/instrumentation/pulsar/__init__.py

Co-authored-by: Rani Mufid <ranimufid@gmail.com>
Co-authored-by: Diego Hurtado <ocelotl@users.noreply.github.com>
Signed-off-by: Jayson Reis <santosdosreis@gmail.com>
Signed-off-by: Jayson Reis <santosdosreis@gmail.com>
Signed-off-by: Jayson Reis <santosdosreis@gmail.com>
Signed-off-by: Jayson Reis <santosdosreis@gmail.com>
…api compatibility

Signed-off-by: Jayson Reis <santosdosreis@gmail.com>
…r better visibility in jaeger and zipkin

Signed-off-by: Jayson Reis <santosdosreis@gmail.com>
Signed-off-by: Jayson Reis <santosdosreis@gmail.com>
Signed-off-by: Jayson Reis <santosdosreis@gmail.com>
Signed-off-by: Jayson Reis <santosdosreis@gmail.com>
JonathanxD and others added 3 commits March 11, 2024 13:15
The `trace.use_span(span, True)` will already close this span,
if we store this same span in the message, it'll be closed
by a call to any of the `*acknowledge` functions, essentially
causing a double call to `span.end()`.

This change prevents this from happening by not storing the
span in the message.
Copy link

CLA Not Signed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants