obsservice: export StatementInsightsStatistics #113498
Conversation
|
This change is |
maryliag
force-pushed
the
export-insights
branch
4 times, most recently
from
45eda49
to
8086fac
Compare
There was a problem hiding this comment.
🎉 🎉 🎉 Looking good!
Just a couple tiny nits
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @maryliag)
pkg/obsservice/obspb/obsservice.proto line 85 at r3 (raw file):
enum Status { Completed = 0;
nit: for these (Status/Problem/Cause), should we prefix the type name with something like "Insight" (e.g. InsightStatus), so that it's more obvious which event type they pertain to? Just thinking about what this file might look like a year in the future 🙂
pkg/sql/sqlstats/insights/registry.go line 200 at r3 (raw file):
func (s *Statement) CopyTo( ctx context.Context, t *Transaction, session Session, other *obspb.StatementInsightsStatistics,
nit: can session be a pointer?
Add proto definition for Statement Insights to be used on Obs Service. Creates function to copy a `insights.Statement` to the new `obspb.StatementInsightsStatistics` format. For this version, there are some parameters not being set, such as contention. Those will be populated on a following iteration. Epic: none Release note: None
This patch hooks into the Flush functionality used by PersistedSQLStats, in preparation for Insights to be sent to external o11y systems. A following PR will do the actual export during the flush. Lastly, it's acknowledged that the transformation required here is likely going to be heavy on allocations. During the prototyping phase however, we leave the optimization and/or restructuring of the exported type for the future. For now, we use a sync.Pool in an effort to reduce allocations/GC. Release note: none
maryliag
force-pushed
the
export-insights
branch
from
8086fac
to
c4d59bb
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/obsservice/obspb/obsservice.proto line 85 at r3 (raw file):
Previously, abarganier (Alex Barganier) wrote…
nit: for these (Status/Problem/Cause), should we prefix the type name with something like "Insight" (e.g. InsightStatus), so that it's more obvious which event type they pertain to? Just thinking about what this file might look like a year in the future 🙂
I decided to move them to inside the Statements Insights definition, so it's clear it's part of it, and when using it it would need to add the prefix anyway
pkg/sql/sqlstats/insights/registry.go line 200 at r3 (raw file):
Previously, abarganier (Alex Barganier) wrote…
nit: can
sessionbe a pointer?
Done
There was a problem hiding this comment.
Reviewable status:
pkg/obsservice/obspb/obsservice.proto line 85 at r3 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
I decided to move them to inside the Statements Insights definition, so it's clear it's part of it, and when using it it would need to add the prefix anyway
Cool, thanks! LGTM
pkg/sql/sqlstats/insights/registry.go line 200 at r3 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
Done
Thanks!
|
TFTR! bors r+ |
|
Build succeeded: |
commit 1:
obsservice: add stmt insights proto and conversion
Add proto definition for Statement Insights to be
used on Obs Service.
Creates function to copy a
insights.Insightto the newobspb.StatementInsightsStatisticsformat.For this version, there are some parameters not being set,
such as contention. Those will be populated on a following
iteration.
Epic: none
Release note: None
commit 2:
obsservice: export StatementInsightsStatistics
This patch hooks into the Flush functionality used
by PersistedSQLStats, in preparation for Insights
to be sent to external o11y systems.
A following PR will do the actual export during the flush.
Lastly, it's acknowledged that the transformation
required here is likely going to be heavy on
allocations. During the prototyping phase however,
we leave the optimization and/or restructuring of
the exported type for the future. For now, we use
a sync.Pool in an effort to reduce allocations/GC.
Release note: none