Skip to content

Conversation

@haihongren
Copy link

Hey guys

I am working for New Relic. I have added the code for the newrelic receiver.
Could you please review the PR and advise me the next step.

Thanks,
Haihong

@haihongren haihongren force-pushed the main branch 2 times, most recently from cf7b479 to 034ef2c Compare June 30, 2021 01:46
@haihongren
Copy link
Author

haihongren commented Jun 30, 2021

Hi @beorn7

I found your name from other PR. Could you please help to review this PR?

It passed all tests except "test_frontend". I think it is related to the change to "template/default.tmpl". Not sure how to fix it on my end. My local test passed successfully.

Thanks in advance!
Haihong

@beorn7
Copy link
Member

beorn7 commented Jun 30, 2021

Thanks for your contribution.

However, I'm not the maintainer of this repo. I helped a bit on a best effort basis, but right now, I have zero time left to do things here.

@simonpasquier could you look at this or delegate?

@haihongren
Copy link
Author

Thanks @beorn7

@simonpasquier Please advise when you have a chance. Thanks!

Copy link
Member

@roidelapluie roidelapluie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would review this later, in the mean time I have added a comment. it also seems that the documentation is completely missing.

@haihongren
Copy link
Author

I would review this later, in the mean time I have added a comment. it also seems that the documentation is completely missing.

@roidelapluie Thanks for pointing out the missing documentation. How do I provide the documentation? in what format?

@roidelapluie
Copy link
Member

In markdown, in the docs/ directory

@haihongren
Copy link
Author

@roidelapluie The New Relic receiver documentation is included in docs/configuration.md now. Thanks!

@stale stale bot added the stale label Oct 16, 2021
@haihongren
Copy link
Author

@roidelapluie One of the customers really wants to have this feature merged into the master so that they could use in production. Could you please review at your earliest? Thanks.

@stale stale bot removed the stale label Oct 26, 2021
@RichiH
Copy link
Member

RichiH commented Oct 27, 2021

As a side note, could you please sign the DCO, @haihongren ?

@haihongren
Copy link
Author

@RichiH Thanks for the comment. DCO signed for the commit.

@roidelapluie
Copy link
Member

is it possible to have a test account at NR to test this?

@haihongren
Copy link
Author

@roidelapluie I have added prometheus-team email address as an NR user. You should receive an email invite at the prometheus-team email account.

@haihongren
Copy link
Author

@roidelapluie Is there anything else you need to proceed with the PR?

@roidelapluie
Copy link
Member

Thanks! I finally have some time to dig into this.

Do you know how I can create an API key that works? I currently get 403 with the key I created.

@roidelapluie
Copy link
Member

I also wonder if we should use the logs api or the incidents API.

@stale stale bot added the stale label Feb 5, 2022
Signed-off-by: haihongren <haihong.ren@gmail.com>
Signed-off-by: haihongren <haihong.ren@gmail.com>
@haihongren haihongren force-pushed the main branch 3 times, most recently from 5266a3e to 68f90b9 Compare July 24, 2023 07:17
@haihongren
Copy link
Author

haihongren commented Jul 24, 2023

I also wonder if we should use the logs api or the incidents API.

@roidelapluie Logs api allows user to collect alerts, generate report/dashboard for alerts, even take further action if required.

I would avoid that and add the ability to pass the license key explicitly. We should not put that burden to users.
@roidelapluie agreed. I removed the Headers setting option. User will use api_key to provide license key explicitly.

Signed-off-by: haihongren <haihong.ren@gmail.com>
@Conejsksk
Copy link

Hi guys, any update on this?

@haihongren
Copy link
Author

Hi, can someone please review the PR? thanks

Copy link
Contributor

@siavashs siavashs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this PR still valid? If yes, please rebase and address the comments.

}, nil
}

type NewRelicCreateMessage struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This type is not exported.

Suggested change
type NewRelicCreateMessage struct {
type newRelicMessage struct {

Comment on lines +40 to +50
- `<duration>`: a duration matching the regular expression `((([0-9]+)y)?(([0-9]+)w)?(([0-9]+)d)?(([0-9]+)h)?(([0-9]+)m)?(([0-9]+)s)?(([0-9]+)ms)?|0)`, e.g. `1d`, `1h30m`, `5m`, `10s`
- `<labelname>`: a string matching the regular expression `[a-zA-Z_][a-zA-Z0-9_]*`
- `<labelvalue>`: a string of unicode characters
- `<filepath>`: a valid path in the current working directory
- `<boolean>`: a boolean that can take the values `true` or `false`
- `<string>`: a regular string
- `<secret>`: a regular string that is a secret, such as a password
- `<tmpl_string>`: a string which is template-expanded before usage
- `<tmpl_secret>`: a string which is template-expanded before usage that is a secret
- `<int>`: an integer value
- `<regex>`: any valid [RE2 regular expression](https://github.com/google/re2/wiki/Syntax) (The regex is anchored on both ends. To un-anchor the regex, use `.*<regex>.*`.)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems unnecessary to change the format of the bullet points.

[ wechat_api_secret: <secret> ]
[ wechat_api_corp_id: <string> ]
[ newrelic_api_key: <secret> ] New Relic Insert API key
[ newrelic_api_url: <string> | default = "https://log-api.newrelic.com/log/v1" ]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This API endpoint seems to be dedicated for sending logs, nothing wrong with logging alerts but is this still a valid API endpoint to send alert messages to or is there now a dedicated endpoint for alerts?

# All alerts that do not match the following child routes
# will remain at the root node and be dispatched to 'default-receiver'.
routes:
# All alerts with service=mysql or service=cassandra
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary reformatting.

Comment on lines -283 to +286
time_intervals:
[ - <time_interval_spec> ... ]
time_intervals: [- <time_interval_spec> ...]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary reformatting.
Not going to point out the rest, make sure to remove them.

Comment on lines +70 to +72
type NewRelicCloseMessage struct {
Source string `json:"source"`
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this needed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants