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

Fix example config for Pulsar exporter, units are nanoseconds #2185

Merged

Conversation

lhotari
Copy link
Contributor

@lhotari lhotari commented Oct 28, 2022

  • setting partitions_auto_discovery_interval to 1 causes a flood of CommandPartitionedTopicMetadata requests to brokers. The broker doesn't log these requests with info level, and it's hard to detect the issue.

  • setting batching_max_publish_delay to 10 will make batching ineffective since the unit is nanoseconds.

- setting partitions_auto_discovery_interval to 1 causes a storm of
  CommandPartitionedTopicMetadata requests to brokers. The broker doesn't log these
  requests and it's hard to detect the issue.
  The default value in the client is 1 minute which is 60*10^9.
  Set the example value to the default.

- setting batching_max_publish_delay to 10 will make batching uneffective
  since the unit is nanoseconds. The default value in the client is 10 milliseconds
  which is 10^7. Set the example value to the default.
Copy link

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

great catch !

@zzzming
Copy link

zzzming commented Oct 28, 2022

That's an excellent finding! Thanks @lhotari

time.Duration is in nano seconds
https://pkg.go.dev/time#Duration

Copy link
Contributor

@pmcollins pmcollins left a comment

Choose a reason for hiding this comment

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

For the record, the Pulsar exporter in this repo was developed for a very narrow use case and is not really intended for general use. As a result, we really have no way to test this, but since this change is for an example config, I think it's low risk. Thanks for fixing.

@pmcollins pmcollins merged commit 5b637ba into signalfx:main Nov 2, 2022
@github-actions github-actions bot mentioned this pull request Dec 2, 2024
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