Repository navigation
fix(spanner): do not export metrics during shutdown if prev export was less than 30 seconds ago - #12266
Conversation
product-auto-label
Bot
added
api: spanner
olavloite
left a comment
olavloite
left a comment
There was a problem hiding this comment.
I think that a better title for this PR would be:
do not export metrics during shutdown if prev export was less than 30 seconds ago
| func (me *monitoringExporter) close() { | ||
| // stop the exporter if last export happens within half-time of default sample period | ||
| if time.Since(me.lastExportedAt) <= (defaultSamplePeriod / 2) { | ||
| me.stop = true |
There was a problem hiding this comment.
Does this field need to be protected by a mutex? That is; could it be that there are two different goroutines that call close() and Export() at the same time?
There was a problem hiding this comment.
It's not possible. close() and Export() can be called in parallel. Export() doesn't change stop value. Two goroutines can't modify the same field.
| errs = append(errs, err) | ||
| } | ||
|
|
||
| me.lastExportedAt = time.Now() |
There was a problem hiding this comment.
Same with this field; does it need to be protected by a mutex?
There was a problem hiding this comment.
Exporter have one goroutine which triggers for every 60 seconds. So I feel it's not required. We will not have any scenario where multiple exports happens in parallel.
There was a problem hiding this comment.
That is true, but that is not what I'm worried about. lastExportedAt is written by this method, which is called by the goroutine handling exports. The field is read by the stop() method, which is called by any application goroutine that closes the Spanner client. Meaning that you could have one goroutine reading the value while another is writing the value.
9a6c5bc to
46f3780
Compare
sakthivelmanii
changed the title
…s less than 30 seconds ago
46f3780 to
fb54b16
Compare
|
|
||
| func percentiles(percentile float32, latencies []int64) any { | ||
| rank := (percentile * float32(len(latencies)-1)) + 1 | ||
| rank := percentile * float32(len(latencies)-1) |
|
|
||
| func (me *monitoringExporter) stop() { | ||
| // stop the exporter if last export happens within half-time of default sample period | ||
| me.mu.Lock() |
There was a problem hiding this comment.
super nit: it is idiomatic to use defer me.mu.Unlock() here
| shutdown chan struct{} | ||
| client *monitoring.MetricClient | ||
| shutdownOnce sync.Once | ||
| mu sync.Mutex |
There was a problem hiding this comment.
nit: Add an empty line here. It is common to create a separate 'group' for the fields that are protected by a mutex. So you get this:
type monitoringExporter struct {
...
shutdownOnce sync.Once
mu sync.Mutex
stopExport bool
lastExportedAt time.Time
}
504438a to
58a71c3
Compare
3 participants
Fixes #12017
Fixes #12091