Skip to content

Refactor http-based alarm plugins and extract common logic to HttpAlarmCallback#10401

Merged
kezhenxu94 merged 1 commit intoapache:masterfrom
kezhenxu94:alarm
Feb 17, 2023
Merged

Refactor http-based alarm plugins and extract common logic to HttpAlarmCallback#10401
kezhenxu94 merged 1 commit intoapache:masterfrom
kezhenxu94:alarm

Conversation

@kezhenxu94
Copy link
Copy Markdown
Member

  • If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #.
  • Update the CHANGES log.

@kezhenxu94 kezhenxu94 added the backend OAP backend related. label Feb 16, 2023
@kezhenxu94 kezhenxu94 added this to the 9.4.0 milestone Feb 16, 2023
@kezhenxu94 kezhenxu94 requested a review from wu-sheng February 16, 2023 15:36
@kezhenxu94 kezhenxu94 marked this pull request as ready for review February 16, 2023 15:36
Comment on lines +41 to +42
.header("Content-Type", "application/json")
.header("Accept", "application/json")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we should expose this. We can't make sure HTTP endpoint using JSON format.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think we should expose this. We can't make sure HTTP endpoint using JSON format.

This is not "make sure", it's a preference here, this means the client (us, OAP) prefers json response, and if the server can provide the response in JSON format, it will send the JSON in high priority, otherwise the server will send whatever format it can. See https://developer.mozilla.org/en-US/docs/Web/HTTP/Content_negotiation

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

you can test this with

curl -H"Accept: application/xml" https://httpbin.org/json

It won't return xml, it will still return json

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I know this is flexible for most time, but I really had seen some notification server has very strict rules to check and parse data.
I just recommend we should have the post method to expose this, and add a postJSON to lock this to be convenient for callbacks.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I just remove this header directly... it's never used in our alarm plugins, and it's seldom used in my opinion, also if in the future we need, we can simply pass in the headers argument of this method.

@kezhenxu94 kezhenxu94 force-pushed the alarm branch 4 times, most recently from 9867d01 to 8cee004 Compare February 16, 2023 16:49
@kezhenxu94
Copy link
Copy Markdown
Member Author

@wu-sheng mind taking another look?

@kezhenxu94 kezhenxu94 merged commit feaf493 into apache:master Feb 17, 2023
@kezhenxu94 kezhenxu94 deleted the alarm branch February 17, 2023 01:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend OAP backend related.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants