Implement the monitoring interface for performance tests #3465
Conversation
| Aws::String unit; | ||
| int64_t date; | ||
| Aws::Vector<int64_t> measurements; | ||
| Aws::Vector<std::pair<Aws::String, Aws::String>> dimensions; |
There was a problem hiding this comment.
should this be a Aws::Map<Aws::String, Aws::String>? would we ever have duplicate keys or want to handle duplicate keys?
There was a problem hiding this comment.
We will not have duplicate keys here. In this case, Aws::Map<Aws::String, Aws::String> can be more efficient.
| Aws::String name; | ||
| Aws::String description; | ||
| Aws::String unit; | ||
| int64_t date; |
There was a problem hiding this comment.
why int64_t instead of Aws::Utils::Datetime? using the datetime data type is likely what we want to do, then extract the time we want from it.
| Aws::String description; | ||
| Aws::String unit; | ||
| int64_t date; | ||
| Aws::Vector<int64_t> measurements; |
There was a problem hiding this comment.
instead of using int64_t for measurements, you may want to make a Measurement type that can have different implementations, so we can support floating point measurements in the future.
| void DumpJson() const; | ||
|
|
||
| mutable Aws::Vector<PerformanceMetricRecord> m_performanceRecords; | ||
| static Aws::Vector<std::pair<Aws::String, Aws::String>> TestDimensions; |
There was a problem hiding this comment.
these should not be static variables to the class, they should be member variables that are assigned in the constructor.
| * Sets test dimensions that will be included with all performance records. | ||
| * @param dimensions Vector of key-value pairs representing test dimensions (e.g., size, bucket type) | ||
| */ | ||
| static void SetTestContext(const Aws::Vector<std::pair<Aws::String, Aws::String>>& dimensions); |
There was a problem hiding this comment.
i comment on this further, but we should avoid creating static state that is shared between instances of the metrics reporter, instead using member variables that are assigned in a constructor.
|
|
||
| void JsonReportingMetrics::SetOutputFilename(const Aws::String& filename) { OutputFilename = filename; } | ||
|
|
||
| JsonReportingMetrics::~JsonReportingMetrics() { DumpJson(); } |
| auto iterator = metricsFromCore.httpClientMetrics.find(latencyKey); | ||
| if (iterator != metricsFromCore.httpClientMetrics.end()) { | ||
| durationMs = iterator->second; | ||
| } |
There was a problem hiding this comment.
if we don't find the record we likely want to fail instead of reporting the metric. is there anyway we can report back to the runner that we have failed to collect metrics instead of reporting 0?
|
|
||
| void* JsonReportingMetrics::OnRequestStarted(const Aws::String&, const Aws::String&, | ||
| const std::shared_ptr<const Aws::Http::HttpRequest>&) const { | ||
| return nullptr; |
There was a problem hiding this comment.
I think we might have a little problem here specifically about how you create durationMs and are tracking it here. and i suggest you look at DefaultMonitoring about how we do it in the default monitor . we need to start the duration count here, and finish it when we succeed. You actually try following the pattern in that file.
Description of changes:
This PR adds performance metrics reporting for SDK service operations. It implements a monitoring interface that collects latency data and outputs it in JSON format.
Check all that applies:
Check which platforms you have built SDK on to verify the correctness of this PR.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.