-
Notifications
You must be signed in to change notification settings - Fork 194
feat(newrelic): APM application id can be looked up by name #361
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
063b8d4 to
2d77a47
Compare
pkg/services/newrelic.go
Outdated
| } | ||
|
|
||
| func (s newrelicService) getApplicationId(client *http.Client, appName string) (string, error) { | ||
| applicationsApi := fmt.Sprintf("%s/v2/applications.json?filter[name]=%s", s.opts.ApiURL, appName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd use the url library to construct this, to avoid appName potentially injecting arbitrary stuff
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pkg/services/newrelic.go
Outdated
| } | ||
| } | ||
| } | ||
| markerApi := fmt.Sprintf(s.opts.ApiURL+"/v2/applications/%s/deployments.json", appId) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, use url package to construct URLs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pkg/services/newrelic.go
Outdated
| var appId = dest.Recipient | ||
| if dest.Recipient != "" { | ||
| _, err := strconv.Atoi(dest.Recipient) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of reusing Recipient, could you use a new RecipientName field? Clarifies the UX and avoids any weirdness around, for example, a name that happens to also be a number.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for taking a look at this. I first interpreted your comment as changing the Destination struct but I'm not actually sure where you mean without making this change of wider significance.
I had a look at the other services to see how they had handled service-specific changes in recipient fields:
notifications-engine/pkg/services/rocketchat.go
Lines 75 to 79 in f189f7e
| // It's a channel | |
| if strings.HasPrefix(dest.Recipient, "#") || strings.HasPrefix(dest.Recipient, "@") { | |
| message.Channel = dest.Recipient | |
| } else { | |
| message.RoomID = dest.Recipient |
notifications-engine/pkg/services/telegram.go
Lines 28 to 45 in f189f7e
| if strings.HasPrefix(dest.Recipient, "-") { | |
| chatID, err := strconv.ParseInt(dest.Recipient, 10, 64) | |
| if err != nil { | |
| return err | |
| } | |
| // Init message with ParseMode is 'Markdown' | |
| msg := tgbotapi.NewMessage(chatID, notification.Message) | |
| msg.ParseMode = "Markdown" | |
| _, err = bot.Send(msg) | |
| if err != nil { | |
| return err | |
| } | |
| } else { | |
| // Init message with ParseMode is 'Markdown' | |
| msg := tgbotapi.NewMessageToChannel("@"+dest.Recipient, notification.Message) | |
| msg.ParseMode = "Markdown" |
Could we do something similar with name: 12345678 and id: 12345678 and default to the latter given no prefix for backwards compatibility? That would leave the Destination struct alone and address the issue of New Relic APM names that happen to be numbers.
|
Hey @3bbbeau could you please address comments? |
0613604 to
cb85658
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #361 +/- ##
==========================================
+ Coverage 55.41% 60.12% +4.71%
==========================================
Files 46 48 +2
Lines 4125 3742 -383
==========================================
- Hits 2286 2250 -36
+ Misses 1511 1131 -380
- Partials 328 361 +33 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This is intended to be a quality of life addition to allow `ApplicationSet` resources to minimize boilerplate when dealing with NewRelic APM resources across multiple environments. - An application programmatically [uses the NR SDK to report to NewRelic](https://github.com/newrelic/go-agent/blob/master/v3/examples/server/main.go#L267-L274) - NewRelic APM Application ID has to then be retrieved following the first run of an application from the NR Dashboard and added to an ArgoCD `Application` via the annotation, across multiple environments. Using ArgoCD `ApplicationSet` resources, the annotation can be either provided via app_id, or by name, e.g. `MyApp-{{ .values.env }}` If `dest.Recipient` can be parsed to an int, then app_id is provided and logic remains as before. If `dest.Recipient` cannot be parsed to an integer, then it was passed by name and we call `/v2/applications.json` to query using `filter[name]` If `/v2/applications.json` returns multiple application IDs then an error is returned, as we can't determine which app_id to use to place the deployment marker. Signed-off-by: 3bbbeau <[email protected]>
Signed-off-by: 3bbbeau <[email protected]>
Guard against potential injection from appName Signed-off-by: 3bbbeau <[email protected]>
cb85658 to
eedfe1e
Compare
- error wrapping %w - http.NoBody over nil - unused parameter 'r' - assert errors with require Signed-off-by: 3bbbeau <[email protected]>
eedfe1e to
1582b2f
Compare
|
@pasha-codefresh Hey, I've addressed all but #361 (comment) which I'm not sure I understand broader implementation details |
Summary
This is intended to be a quality of life feature to allow ArgoCD
ApplicationSetresources to minimize boilerplate when dealing with NewRelic APM resources across multiple environments.Currently
Applicationvia the annotation, across multiple environments.Desired
Using ArgoCD
ApplicationSetresources, the annotation can be either provided via the application id as the above example, or instead by name, e.g.MyApp-{{ .values.env }}:Logic
If
dest.Recipientcan be parsed to an int, then the application id was directly provided and logic remains as before.If
dest.Recipientcannot be parsed to an integer, then it was passed by name (since all NewRelic APM IDs are defined as integers within the schema) and we call/v2/applications.jsonto query usingfilter[name]If
/v2/applications.jsonreturns multiple application IDs then an error is returned, as we can't determine which application id to use to place the deployment marker.