DOCSP-46228: Add Scaling example script to Go SDK project #73
Conversation
dacharyc
left a comment
dacharyc
left a comment
There was a problem hiding this comment.
Overall LGTM - just a few non-blocking Qs, and I presume we should remove the binary before we merge it.
There was a problem hiding this comment.
I assume we don't want to commit a binary here?
There was a problem hiding this comment.
no clue how that happened. but no, we don't 😅
| ATLAS_DOWNLOADS_DIR="tmp/atlas_downloads" # optional download directory | ||
| CONFIG_PATH="configs/config.development.json" # optional path to Atlas config file | ||
| # Programmatic scaling settings |
There was a problem hiding this comment.
Hmm. 🤔
I'm not sure where the right place would be, but I wonder if these settings should be somewhere other than the .env file. We say "Create a .env file... with your MongoDB Atlas service account credentials" - but this is kind of becoming a catch-all for vars that are only needed for some subset of the examples.
I wonder if examples should have their own file(s) to specify example-specific settings, so only the ones that are needed for multiple examples are specified in the top-level config?
Maybe I'm overthinking it - just wanted to throw this out as a Q.
For example, if I'm here to check out billing I don't necessarily want to think about why I'm being asked to add a bunch of programmatic scaling vars to my .env, and whether those apply to my use case.
There was a problem hiding this comment.
I've moved it to the config as a new stanza
| "time" | ||
|
|
||
| "atlas-sdk-go/internal/auth" | ||
| clusterutils "atlas-sdk-go/internal/clusters" |
There was a problem hiding this comment.
Q: should we rename clusters to clusterutils so we don't have to use an import alias? It's different than how we're using the other internal imports so just had to ask.
There was a problem hiding this comment.
👍 renamed the package, and renamed the file so it's more descriptive
| PreScale: strings.EqualFold(strings.TrimSpace(os.Getenv("PRE_SCALE_EVENT")), "false"), | ||
| CPUThreshold: 75.0, | ||
| PeriodMinutes: 60, | ||
| DryRun: strings.EqualFold(strings.TrimSpace(os.Getenv("DRY_RUN")), "false"), |
There was a problem hiding this comment.
Should we default to "DRY_RUN" = true as a safer default for executing this example?
| "github.com/joho/godotenv" | ||
| ) | ||
|
|
||
| func main() { |
There was a problem hiding this comment.
Curious if you were able to run this successfully to confirm it works.
When I try running it against Bushicorp in dry-run mode, I get this output:
Found 2 clusters to analyze for scaling
=== Analyzing cluster: Cluster0 ===
- Current tier: M10, Target tier: M50
Warning: unable to compute average CPU for reactive scaling: no process found for cluster Cluster0
- Conditions not met: metrics unavailable for reactive scaling decision
=== Analyzing cluster: AtlasCluster ===
- Current tier: M0, Target tier: M50
Warning: unable to compute average CPU for reactive scaling: no process found for cluster AtlasCluster
- Conditions not met: metrics unavailable for reactive scaling decisionSo it's seeing the clusters but says metrics are unavailable. I don't know if that's because the clusters may be paused or inactive, or if there's a logic problem. Have you been able to see this working in your testing?
There was a problem hiding this comment.
this was modified a bit, but it should be working for you now. lmk if you're still having issues
There was a problem hiding this comment.
Q - should we add configs/config.production.json and configs/config.dev.json et al to the .gitignore? Whenever I create one of these files, JetBrains aggressively asks me if I want to commit it and I wonder if we should be guarding against that as a best practice.
…te gitignore, rename clusters pkg, fix scale example for shared clusters
2 participants
scaling/main.goto demonstrate how to programmatically pre-scale a cluster or scale based on specific criteriainternal/scale/