Skip to content

fix(metrics): Fix metrics propagation for destination plugins with managed write mode#583

Closed
candiduslynx wants to merge 0 commit intomainfrom
fix/destinations/metrics
Closed

fix(metrics): Fix metrics propagation for destination plugins with managed write mode#583
candiduslynx wants to merge 0 commit intomainfrom
fix/destinations/metrics

Conversation

@candiduslynx
Copy link
Copy Markdown
Contributor

No description provided.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Jan 8, 2023

⏱️ Benchmark results

Comparing with 7c15d9a

  • DefaultConcurrencyDFS-2 resources/s: 10,749 ⬇️ 5.94% decrease vs. 7c15d9a
  • DefaultConcurrencyRoundRobin-2 resources/s: 12,301 ⬆️ 9.08% increase vs. 7c15d9a
  • Glob-2 ns/op: 185.1 ⬆️ 20.85% increase vs. 7c15d9a
  • TablesWithChildrenDFS-2 resources/s: 28,704 ⬇️ 5.68% decrease vs. 7c15d9a
  • TablesWithChildrenRoundRobin-2 resources/s: 28,237 ⬇️ 8.16% decrease vs. 7c15d9a
  • TablesWithRateLimitingDFS-2 resources/s: 28.27 (no change)
  • TablesWithRateLimitingRoundRobin-2 resources/s: 857.8 ⬆️ 1.47% increase vs. 7c15d9a
  • BufferedScanner-2 ns/op: 11.87 ⬆️ 13.48% increase vs. 7c15d9a
  • LogReader-2 ns/op: 35.23 ⬆️ 13.45% increase vs. 7c15d9a

Copy link
Copy Markdown
Member

@erezrokah erezrokah 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 PR @candiduslynx, can we separate the refactor from the bug fix?
It's hard to understand the fix itself.

Also I think with this fix, we're losing the granularity of the error messages (e.g. which flush failed)

@candiduslynx candiduslynx force-pushed the fix/destinations/metrics branch from f4d14d4 to b693034 Compare January 9, 2023 10:15
@candiduslynx candiduslynx deleted the fix/destinations/metrics branch January 9, 2023 10:15
@candiduslynx
Copy link
Copy Markdown
Contributor Author

refactor part was similarly implemented in #582.

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.

2 participants