This PR is part of a series: see the prototype https://github.com/palantir/atlasdb/pull/7000 or the internal RFC for what all of the pieces together are expected to look like. I have tried to separate this feature into reasonably sized components as otherwise it'd probably have a 5k delta or so!
Before this PR: SnapshotTransaction is responsible for its own metrics, including creating timers/counters and ticking them appropriately.
After this PR:
==COMMIT_MSG==
Metrics on the read protocol paths in SnapshotTransaction now go through a KeyValueSnapshotEventRecorder. Eventually, when a KeyValueSnapshotReader is separated out, the reader will ONLY log metrics through the KeyValueSnapshotEventRecorder, and not through direct access to TransactionMetrics or the MetricsManager/Registry.
Write path metrics still go through the MetricRegistry and so on directly, though this is extracted to a MetricFactory layer.
==COMMIT_MSG==
Priority: High P2; nothing special but high priority workstream
Concerns / possible downsides (what feedback would you like?):
Is KeyValueSnapshotEventRecorder a good name / does it sound like it might produce event logs rather than metrics? Is MetricRecorder or MetricUpdater a better name?
I opted to leave the origin for the metrics to be under SnapshotTransaction in order to not break existing dashboards using these metrics. Is this more confusing than it's worth?
Is documentation needed?: No.
Compatibility
Does this PR create any API breaks (e.g. at the Java or HTTP layers) - if so, do we have compatibility?: No
Does this PR change the persisted format of any data - if so, do we have forward and backward compatibility?: No
The code in this PR may be part of a blue-green deploy. Can upgrades from previous versions safely coexist? (Consider restarts of blue or green nodes.): Yes
Does this PR rely on statements being true about other products at a deployment - if so, do we have correct product dependencies on these products (or other ways of verifying that these statements are true)?: No
Does this PR need a schema migration? No
Testing and Correctness
What, if any, assumptions are made about the current state of the world? If they change over time, how will we find out?: Nothing in particular
What was existing testing like? What have you done to improve it?: Not much. Relying on existing tests here, and the code is fairly simple.
If this PR contains complex concurrent or asynchronous code, is it correct? The onus is on the PR writer to demonstrate this.: N/A
If this PR involves acquiring locks or other shared resources, how do we ensure that these are always released?: N/A
Execution
How would I tell this PR works in production? (Metrics, logs, etc.): Metrics continue to work
Has the safety of all log arguments been decided correctly?: N/A
Will this change significantly affect our spending on metrics or logs?: No
How would I tell that this PR does not work in production? (monitors, etc.): Metrics no longer work properly
If this PR does not work as expected, how do I fix that state? Would rollback be straightforward?: Rollback
If the above plan is more complex than “recall and rollback”, please tag the support PoC here (if it is the end of the week, tag both the current and next PoC):
Scale
Would this PR be expected to pose a risk at scale? Think of the shopping product at our largest stack.: No
Would this PR be expected to perform a large number of database calls, and/or expensive database calls (e.g., row range scans, concurrent CAS)?: No
Would this PR ever, with time and scale, become the wrong thing to do - and if so, how would we know that we need to do something differently?: I don't think so
Development Process
Where should we start reviewing?: KeyValueSnapshotEventRecorder
If this PR is in excess of 500 lines excluding versions lock-files, why does it not make sense to split it?:
Please tag any other people who should be aware of this PR:
@jeremyk-91
@sverma30
@raiju
General
This PR is part of a series: see the prototype https://github.com/palantir/atlasdb/pull/7000 or the internal RFC for what all of the pieces together are expected to look like. I have tried to separate this feature into reasonably sized components as otherwise it'd probably have a 5k delta or so!
Before this PR: SnapshotTransaction is responsible for its own metrics, including creating timers/counters and ticking them appropriately.
After this PR:
==COMMIT_MSG== Metrics on the read protocol paths in SnapshotTransaction now go through a KeyValueSnapshotEventRecorder. Eventually, when a KeyValueSnapshotReader is separated out, the reader will ONLY log metrics through the KeyValueSnapshotEventRecorder, and not through direct access to TransactionMetrics or the MetricsManager/Registry.
Write path metrics still go through the MetricRegistry and so on directly, though this is extracted to a MetricFactory layer. ==COMMIT_MSG==
Priority: High P2; nothing special but high priority workstream
Concerns / possible downsides (what feedback would you like?):
KeyValueSnapshotEventRecordera good name / does it sound like it might produce event logs rather than metrics? IsMetricRecorderorMetricUpdatera better name?SnapshotTransactionin order to not break existing dashboards using these metrics. Is this more confusing than it's worth?Is documentation needed?: No.
Compatibility
Does this PR create any API breaks (e.g. at the Java or HTTP layers) - if so, do we have compatibility?: No
Does this PR change the persisted format of any data - if so, do we have forward and backward compatibility?: No
The code in this PR may be part of a blue-green deploy. Can upgrades from previous versions safely coexist? (Consider restarts of blue or green nodes.): Yes
Does this PR rely on statements being true about other products at a deployment - if so, do we have correct product dependencies on these products (or other ways of verifying that these statements are true)?: No
Does this PR need a schema migration? No
Testing and Correctness
What, if any, assumptions are made about the current state of the world? If they change over time, how will we find out?: Nothing in particular
What was existing testing like? What have you done to improve it?: Not much. Relying on existing tests here, and the code is fairly simple.
If this PR contains complex concurrent or asynchronous code, is it correct? The onus is on the PR writer to demonstrate this.: N/A
If this PR involves acquiring locks or other shared resources, how do we ensure that these are always released?: N/A
Execution
How would I tell this PR works in production? (Metrics, logs, etc.): Metrics continue to work
Has the safety of all log arguments been decided correctly?: N/A
Will this change significantly affect our spending on metrics or logs?: No
How would I tell that this PR does not work in production? (monitors, etc.): Metrics no longer work properly
If this PR does not work as expected, how do I fix that state? Would rollback be straightforward?: Rollback
If the above plan is more complex than “recall and rollback”, please tag the support PoC here (if it is the end of the week, tag both the current and next PoC):
Scale
Would this PR be expected to pose a risk at scale? Think of the shopping product at our largest stack.: No
Would this PR be expected to perform a large number of database calls, and/or expensive database calls (e.g., row range scans, concurrent CAS)?: No
Would this PR ever, with time and scale, become the wrong thing to do - and if so, how would we know that we need to do something differently?: I don't think so
Development Process
Where should we start reviewing?: KeyValueSnapshotEventRecorder
If this PR is in excess of 500 lines excluding versions lock-files, why does it not make sense to split it?:
Please tag any other people who should be aware of this PR: @jeremyk-91 @sverma30 @raiju