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(sql): table_writer_metrics() and memory_metrics() for better observability #2318

Merged
merged 14 commits into from
Aug 24, 2022

Conversation

jerrinot
Copy link
Contributor

@jerrinot jerrinot commented Jul 13, 2022

It allows to query:

  1. table writer metrics expose these: TOTAL_COMMITS, O3_COMMITS, ROLLBACKS, COMMITTED_ROWS, PHYSICALLY_WRITTEN_ROWS

  2. memory utilization metrics: with a breakdown per tag.

For #1 to work metrics have to be enabled in QuestDB configuration. #2 is always enabled.

Questions I'd like reviewers to think about:

  1. This is effectively API. Are we OK to maintain compatibility? Is it ok to break it? Imagine we remove a memory tag.
  2. Should we enable metrics by default? I feel stats on O3 could be pretty useful for troubleshooting
  3. each table emits just a single row. more over table metrics are reset on QuestDB restart? is that OK? Alternatively, we could expose memory metrics as tuples where each row would represent a (key-value) pair. This would allow simple sorting by memory consumption.

@jerrinot jerrinot changed the title wip: table_write_metrics() chore(sql): table_write_metrics() Jul 13, 2022
@jerrinot jerrinot changed the title chore(sql): table_write_metrics() chore(sql): table_writer_metrics() Jul 14, 2022
@jerrinot jerrinot changed the title chore(sql): table_writer_metrics() chore(sql): table_writer_metrics() and memory_metrics() for better observability Jul 14, 2022
@jerrinot jerrinot marked this pull request as ready for review July 14, 2022 16:20
@jerrinot
Copy link
Contributor Author

jerrinot commented Jul 18, 2022

Please don't merge it yet, there is allocation going on due to auto-boxing. thanks to Vlad for spotting it!
edit: boxing is gone.

@jerrinot jerrinot marked this pull request as draft July 18, 2022 07:40
@jerrinot jerrinot marked this pull request as ready for review August 22, 2022 14:57
@puzpuzpuz puzpuzpuz added Enhancement Enhance existing functionality SQL Issues or changes relating to SQL execution labels Aug 23, 2022
@puzpuzpuz
Copy link
Contributor

  • This is effectively API. Are we OK to maintain compatibility? Is it ok to break it? Imagine we remove a memory tag.

I'd say that we should clearly state in the docs that memory tags (and any metrics) are subject to change.

  • Should we enable metrics by default? I feel stats on O3 could be pretty useful for troubleshooting

+1 from me for enabling metrics by default.

  • each table emits just a single row. more over table metrics are reset on QuestDB restart? is that OK? Alternatively, we could expose memory metrics as tuples where each row would represent a (key-value) pair. This would allow simple sorting by memory consumption.

I'd suggest returning tuples instead of columns. This would make any integration simpler and less coupled with our tags and metrics. WDYT?

@jerrinot
Copy link
Contributor Author

@puzpuzpuz I agree with everything you wrote! I will change the format to return tuples and later send a separate PR to enable metrics by default.

also: scope metrics table functions in the sys. pseudo-schema to avoid polluting global function namespace.
@jerrinot
Copy link
Contributor Author

@puzpuzpuz can you please re-check?

@ideoma
Copy link
Collaborator

ideoma commented Aug 23, 2022

[PR Coverage check]

😍 pass : 91 / 94 (96.81%)

file detail

path covered line new line coverage
🔵 io/questdb/griffin/engine/table/StringLongTuplesRecordCursor.java 26 29 89.66%
🔵 io/questdb/cairo/TableWriterMetrics.java 4 4 100.00%
🔵 io/questdb/griffin/engine/table/TableWriterMetricsRecordCursorFactory.java 32 32 100.00%
🔵 io/questdb/griffin/engine/functions/table/MemoryMetricsFunctionFactory.java 3 3 100.00%
🔵 io/questdb/griffin/engine/table/MemoryMetricsRecordCursorFactory.java 22 22 100.00%
🔵 io/questdb/std/MemoryTag.java 1 1 100.00%
🔵 io/questdb/griffin/engine/functions/table/TableWriterMetricsFunctionFactory.java 3 3 100.00%

Copy link
Collaborator

@ideoma ideoma left a comment

Choose a reason for hiding this comment

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

I'd prefer a function for all prometheus metrics, all in one, not just writer and memory separately.

@ideoma ideoma merged commit e4338af into master Aug 24, 2022
@jerrinot jerrinot deleted the features/tablemetric-observability branch August 24, 2022 13:30
@jerrinot jerrinot changed the title chore(sql): table_writer_metrics() and memory_metrics() for better observability feat(sql): table_writer_metrics() and memory_metrics() for better observability Aug 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Enhance existing functionality SQL Issues or changes relating to SQL execution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants