sc-13403 Added timezone parameter #4
Conversation
main.go
Outdated
| @@ -22,6 +22,7 @@ type Config struct { | |||
| SendGridAccounts []struct { | |||
| AccountName string `yaml:"account_name"` | |||
| APIKey string `yaml:"api_key"` | |||
| TimeZone string `yaml:"time_zone"` // Added TimeZone field | |||
There was a problem hiding this comment.
chore: this comment should be removed.
Comments should only describe things that are not obvious from reading the code.
| TimeZone string `yaml:"time_zone"` // Added TimeZone field | |
| TimeZone string `yaml:"time_zone"` |
There was a problem hiding this comment.
I would additionally suggest sweeping over this code again and removing all such code comments, even if they were merged in last time.
main.go
Outdated
| } | ||
| sendgridExporter := sendgrid.NewExporter(apiKeys) | ||
| sendgridExporter := sendgrid.NewExporter(apiKeys, timeZones) // Passed timeZones to Exporter |
There was a problem hiding this comment.
suggestion: I am thinking that a better interface would be to take multiple structs of configs as opposed to multiple maps where the ordering of the maps needs to be consistent.
confer: Data Clumps
sendgrid/exporter.go
Outdated
| if !exists { | ||
| timeZone = time.UTC // Default to UTC if the time zone is not provided | ||
| } |
There was a problem hiding this comment.
quibble: The config reading code guaranteed a default value of UTC, this is redundant.
But if we refactor away the data clump, this whole thing wouldn't exist anyway. We would be able to access the timezone with account.TimeZone.
sendgrid/exporter.go
Outdated
|
|
||
| log.Printf("timeUntilExpiration: %+v", timeUntilExpiration) |
There was a problem hiding this comment.
question: why are we logging this?
Gyan-Gupta-Rtsl
force-pushed
the
sc-13403
branch
from
3a83e31 to
17f5176
Compare
sendgrid/exporter.go
Outdated
| client: NewClient(apiKeys), | ||
| client: NewClient(apiKeys), // Pass apiKeys map directly | ||
| timeZones: timeZones, // Pass timeZones map |
There was a problem hiding this comment.
chore: this comment should be removed.
reason: comments should only describe things that are not obvious from reading the code.
sendgrid/exporter_test.go
Outdated
| timeZones := map[string]*time.Location{ | ||
| "mockAccount": time.UTC, | ||
| } | ||
| exporter := NewExporter(accountNames, timeZones) |
There was a problem hiding this comment.
issue: This test case won't compile, because the NewExporter function only takes one argument.
Please ensure that all the test cases pass before marking the code for review.
Story_card: https://app.shortcut.com/simpledotorg/story/13403/minor-fixes-in-sendgrid-prometheus-exporter
What:
Added timezone to the exporter
Why:
As ethiopia_production and production(for other countries) has different timezones in their respective sendgrid accounts,
so to get the correct data to monitor in prometheus.