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.

Sign up for GitHub

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

Jump to bottom

Add Starlark script for outputting Line Protocol shape...for sizing #8852

Merged
merged 6 commits into from
Mar 2, 2021
Merged

Add Starlark script for outputting Line Protocol shape...for sizing #8852

merged 6 commits into from
Mar 2, 2021

Conversation

samhld
Copy link
Contributor

@samhld samhld commented Feb 12, 2021

Required for all PRs:

  • Associated README.md updated.
  • Has appropriate unit tests.

telegraf-tiger[bot]
telegraf-tiger bot requested changes Feb 12, 2021
View reviewed changes
Copy link
Contributor

@telegraf-tiger telegraf-tiger bot left a comment

Choose a reason for hiding this comment

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

Thanks so much for the pull request!
🤝 ✒️ Just a reminder that the CLA has not yet been signed, and we'll need it before merging. Please sign the CLA when you get a chance, then post a comment here saying !signed-cla

@telegraf-tiger telegraf-tiger bot added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label Feb 12, 2021
@srebhan srebhan self-assigned this Feb 16, 2021
@srebhan
Copy link
Contributor

srebhan commented Feb 16, 2021

@samhld how is this PR different from #8675?

@samhld
Copy link
Contributor Author

samhld commented Feb 16, 2021

@srebhan should be good to go now!

srebhan
srebhan reviewed Feb 17, 2021
View reviewed changes
Copy link
Contributor

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

Please remove the commented code.

Furthermore, you drop the original metric, is this correct?

@samhld
Copy link
Contributor Author

samhld commented Feb 17, 2021

@srebhan Removed commented code. The intention was to drop the original metric, yes. That's up for debate currently, however :). For now, it's fine.

@srebhan
Copy link
Contributor

srebhan commented Feb 17, 2021

@samhld please add at least a comment in the example that the original metric is dropped. Furthermore, your expected values and the actual output of the script diverge. Please check what's going on here and fix one or the other.

@samhld
Copy link
Contributor Author

samhld commented Feb 17, 2021

Ugh, they didn't use to. I'll have to investigate why this happened.

@samhld
Copy link
Contributor Author

samhld commented Feb 17, 2021

I know why -- will re-commit fix shortly!

@samhld
Copy link
Contributor Author

samhld commented Feb 17, 2021

@srebhan Ok gtg

srebhan
srebhan approved these changes Feb 18, 2021
View reviewed changes
Copy link
Contributor

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

Fine with me.

@srebhan srebhan added the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label Feb 18, 2021
@ssoroka ssoroka merged commit 30a0fd0 into influxdata:master Mar 2, 2021
arstercz pushed a commit to arstercz/telegraf that referenced this pull request Mar 5, 2023
@samhld
Add Starlark script for estimating Line Protocol cardinality (influxd…
6eb25fd 
…ata#8852)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@telegraf-tiger telegraf-tiger[bot] telegraf-tiger requested changes

@srebhan srebhan srebhan approved these changes

Assignees

@srebhan srebhan

Labels
feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@samhld @srebhan @ssoroka