You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by "wankai123 (via GitHub)" <gi...@apache.org> on 2026/08/05 07:33:20 UTC
[PR] Make the PagerDuty Events API v2 endpoint configurable. [skywalking]
wankai123 opened a new pull request, #13968:
URL: https://github.com/apache/skywalking/pull/13968
And make the endpoint default to https://events.pagerduty.com/v2/enqueue.
- [ ] If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #<issue number>.
- [X] Update the [`CHANGES` log](https://github.com/apache/skywalking/blob/master/docs/en/changes/changes.md).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make the PagerDuty Events API v2 endpoint configurable. [skywalking]
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng merged PR #13968:
URL: https://github.com/apache/skywalking/pull/13968
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make the PagerDuty Events API v2 endpoint configurable. [skywalking]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13968:
URL: https://github.com/apache/skywalking/pull/13968#discussion_r3718867304
##########
oap-server/server-alarm-plugin/src/test/java/org/apache/skywalking/oap/server/core/alarm/provider/pagerduty/PagerDutyHookCallbackTest.java:
##########
@@ -24,14 +24,88 @@
import org.apache.skywalking.oap.server.core.alarm.provider.AlarmRulesWatcher;
import org.apache.skywalking.oap.server.core.alarm.provider.Rules;
import org.apache.skywalking.oap.server.core.source.DefaultScopeDefine;
+import com.google.gson.Gson;
+import com.google.gson.JsonObject;
+import com.linecorp.armeria.common.HttpResponse;
+import com.linecorp.armeria.common.HttpStatus;
+import com.linecorp.armeria.server.ServerBuilder;
+import com.linecorp.armeria.testing.junit5.server.ServerExtension;
+import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Disabled;
import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.RegisterExtension;
import java.util.Arrays;
import java.util.List;
+import java.util.concurrent.CopyOnWriteArrayList;
public class PagerDutyHookCallbackTest {
+ private static final List<JsonObject> RECEIVED = new CopyOnWriteArrayList<>();
+
+ @RegisterExtension
+ public static final ServerExtension SERVER = new ServerExtension() {
+ @Override
+ protected void configure(ServerBuilder sb) {
+ sb.service("/v2/enqueue", (ctx, req) -> HttpResponse.from(
+ req.aggregate().thenApply(r -> {
+ RECEIVED.add(new Gson().fromJson(r.contentUtf8(), JsonObject.class));
+ return HttpResponse.of(HttpStatus.OK);
+ })
+ ));
+ }
+ };
+
+ /**
+ * The endpoint shipped as {@code http://} from 9.2.0 until this guard was added, putting the integration key —
+ * which {@code getMessageBody} places in the request body — in the clear on every alarm.
+ */
+ @Test
+ public void defaultEndpointMustUseTls() {
+ Assertions.assertTrue(
+ PagerDutySettings.DEFAULT_EVENTS_API_URL.startsWith("https://"),
+ "PagerDuty endpoint must use https, the routing key is sent in the request body, but was: "
+ + PagerDutySettings.DEFAULT_EVENTS_API_URL
+ );
+ PagerDutySettings settings = new PagerDutySettings("s", AlarmHooksType.pagerduty, true);
+ Assertions.assertEquals(
+ PagerDutySettings.DEFAULT_EVENTS_API_URL, settings.getEventsApiUrl(),
+ "a hook that does not configure events-api-url must fall back to the TLS default"
+ );
+ }
+
+ @Test
+ public void testDoAlarmPostsToConfiguredEndpoint() throws Exception {
+ RECEIVED.clear();
+ Rules rules = new Rules();
+ PagerDutySettings setting = new PagerDutySettings("setting1", AlarmHooksType.pagerduty, true);
+ setting.setEventsApiUrl("http://127.0.0.1:" + SERVER.httpPort() + "/v2/enqueue");
+ setting.setIntegrationKeys(Arrays.asList("key-1", "key-2"));
+ setting.setTextTemplate("Apache SkyWalking Alarm: \\n %s.");
+ rules.getPagerDutySettingsMap().put(setting.getFormattedName(), setting);
+
+ AlarmMessage alarmMessage = new AlarmMessage();
+ alarmMessage.setScopeId(DefaultScopeDefine.SERVICE);
+ alarmMessage.setRuleName("service_resp_time_rule");
+ alarmMessage.setAlarmMessage("alarmMessage with [DefaultScopeDefine.All]");
+ alarmMessage.getHooks().add(setting.getFormattedName());
+
+ new PagerDutyHookCallback(new AlarmRulesWatcher(rules, null, null))
+ .doAlarm(new ArrayList<>(List.of(alarmMessage)));
+
+ // one request per integration key
+ Assertions.assertEquals(2, RECEIVED.size());
+ List<String> routingKeys = new ArrayList<>();
Review Comment:
`ArrayList` is used but not imported in the updated import list shown in this diff. Add `import java.util.ArrayList;` (or use a fully-qualified `java.util.ArrayList`) to avoid a compilation failure.
##########
oap-server/server-alarm-plugin/src/test/java/org/apache/skywalking/oap/server/core/alarm/provider/pagerduty/PagerDutyHookCallbackTest.java:
##########
@@ -24,14 +24,88 @@
import org.apache.skywalking.oap.server.core.alarm.provider.AlarmRulesWatcher;
import org.apache.skywalking.oap.server.core.alarm.provider.Rules;
import org.apache.skywalking.oap.server.core.source.DefaultScopeDefine;
+import com.google.gson.Gson;
+import com.google.gson.JsonObject;
+import com.linecorp.armeria.common.HttpResponse;
+import com.linecorp.armeria.common.HttpStatus;
+import com.linecorp.armeria.server.ServerBuilder;
+import com.linecorp.armeria.testing.junit5.server.ServerExtension;
+import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Disabled;
import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.RegisterExtension;
import java.util.Arrays;
import java.util.List;
+import java.util.concurrent.CopyOnWriteArrayList;
public class PagerDutyHookCallbackTest {
+ private static final List<JsonObject> RECEIVED = new CopyOnWriteArrayList<>();
+
+ @RegisterExtension
+ public static final ServerExtension SERVER = new ServerExtension() {
+ @Override
+ protected void configure(ServerBuilder sb) {
+ sb.service("/v2/enqueue", (ctx, req) -> HttpResponse.from(
+ req.aggregate().thenApply(r -> {
+ RECEIVED.add(new Gson().fromJson(r.contentUtf8(), JsonObject.class));
+ return HttpResponse.of(HttpStatus.OK);
+ })
+ ));
+ }
+ };
+
+ /**
+ * The endpoint shipped as {@code http://} from 9.2.0 until this guard was added, putting the integration key —
+ * which {@code getMessageBody} places in the request body — in the clear on every alarm.
+ */
+ @Test
+ public void defaultEndpointMustUseTls() {
+ Assertions.assertTrue(
+ PagerDutySettings.DEFAULT_EVENTS_API_URL.startsWith("https://"),
+ "PagerDuty endpoint must use https, the routing key is sent in the request body, but was: "
+ + PagerDutySettings.DEFAULT_EVENTS_API_URL
+ );
+ PagerDutySettings settings = new PagerDutySettings("s", AlarmHooksType.pagerduty, true);
+ Assertions.assertEquals(
+ PagerDutySettings.DEFAULT_EVENTS_API_URL, settings.getEventsApiUrl(),
+ "a hook that does not configure events-api-url must fall back to the TLS default"
+ );
+ }
+
+ @Test
+ public void testDoAlarmPostsToConfiguredEndpoint() throws Exception {
+ RECEIVED.clear();
+ Rules rules = new Rules();
+ PagerDutySettings setting = new PagerDutySettings("setting1", AlarmHooksType.pagerduty, true);
+ setting.setEventsApiUrl("http://127.0.0.1:" + SERVER.httpPort() + "/v2/enqueue");
+ setting.setIntegrationKeys(Arrays.asList("key-1", "key-2"));
+ setting.setTextTemplate("Apache SkyWalking Alarm: \\n %s.");
+ rules.getPagerDutySettingsMap().put(setting.getFormattedName(), setting);
+
+ AlarmMessage alarmMessage = new AlarmMessage();
+ alarmMessage.setScopeId(DefaultScopeDefine.SERVICE);
+ alarmMessage.setRuleName("service_resp_time_rule");
+ alarmMessage.setAlarmMessage("alarmMessage with [DefaultScopeDefine.All]");
+ alarmMessage.getHooks().add(setting.getFormattedName());
+
+ new PagerDutyHookCallback(new AlarmRulesWatcher(rules, null, null))
+ .doAlarm(new ArrayList<>(List.of(alarmMessage)));
Review Comment:
`ArrayList` is used but not imported in the updated import list shown in this diff. Add `import java.util.ArrayList;` (or use a fully-qualified `java.util.ArrayList`) to avoid a compilation failure.
##########
oap-server/server-starter/src/main/resources/alarm-settings.yml:
##########
@@ -99,6 +99,9 @@ hooks:
# default:
# is-default: true
# text-template: "Apache SkyWalking Alarm: \n %s."
+# # # Events API v2 endpoint. Defaults to the US service region; set the EU endpoint
+# # # (https://events.eu.pagerduty.com/v2/enqueue) if your PagerDuty account is in the EU region.
Review Comment:
These comment lines have a duplicated comment prefix pattern (`# # #`) that looks like an accidental copy/paste and is inconsistent with surrounding commented examples. Simplify to a single `#` comment prefix (keeping indentation consistent with the rest of the PagerDuty hook example) so the sample configuration is easier to read.
##########
oap-server/server-alarm-plugin/src/main/java/org/apache/skywalking/oap/server/core/alarm/provider/RulesReader.java:
##########
@@ -399,6 +401,19 @@ private void readPagerDutyConfig(Map hooks, Rules rules) {
Object recoveryTextTemplate = config.getOrDefault("recovery-text-template", "");
settings.setRecoveryTextTemplate((String) recoveryTextTemplate);
+ Object eventsApiUrl = config.get("events-api-url");
+ if (eventsApiUrl != null && StringUtil.isNotBlank(eventsApiUrl.toString())) {
+ settings.setEventsApiUrl(eventsApiUrl.toString().trim());
+ }
Review Comment:
The new `events-api-url` value is accepted as an arbitrary string here, but later `PagerDutyHookCallback` calls `URI.create(setting.getEventsApiUrl())`, which will throw `IllegalArgumentException` for invalid URLs and break alarm delivery at runtime. Consider validating/parsing the URL during config load (fail fast with a clear log/exception, or fall back to `DEFAULT_EVENTS_API_URL` when invalid) so misconfiguration is surfaced at startup rather than during alarm sending.
##########
docs/en/changes/changes.md:
##########
@@ -341,6 +341,9 @@
* Support BanyanDB's group-scoped trace retention pipeline in `bydb.yml`. The `trace` and `zipkinTrace` groups gain a `pipeline` block (`enabled`, `enabledEvents`, `mergeGraceSeconds`, `finalizeGraceSeconds`, and an ordered `plugins` chain) that OAP pushes onto the BanyanDB group as a `TracePipelineConfig`, letting a sampler plugin drop traces inside the data node during Hot-phase compaction — after storage, so it reclaims space already written and decides per whole trace, unlike the ingest-side [server-side trace sampling](../setup/backend/trace-sampling.md). Disabled by default, since it deletes stored traces. Even when enabled it is inert unless the data node runs the plugin-capable BanyanDB image with the sampler `.so` mounted — a node without that support ignores the config, and one that cannot load the plugin logs an error and merges unfiltered, so nothing is dropped unexpectedly. The two grace windows use `-1` for "inherit the data node default" (30s merge / 5m finalize)
because the node treats any non-positive grace as unset. `enabledEvents` accepts a comma-separated string so it can be set from the environment (`SW_STORAGE_BANYANDB_TRACE_PIPELINE_ENABLED_EVENTS` and its `ZIPKIN_` variant) as well as a YAML block list; an empty value falls back to `PIPELINE_EVENT_MERGE`, so `PIPELINE_EVENT_FINALIZE` runs only when named explicitly. Each plugin's `config` is passed through verbatim as a protobuf `Struct`: nested lists and objects (e.g. `keepTagRules`) now survive the config loader and are serialized as real `ListValue`/`Struct` rather than being flattened to a string. Note a float written as a `${ENV:default}` placeholder still reaches the plugin as a JSON string, because the shared placeholder resolver only preserves String/Integer/Long/Boolean; the first-party samplers accept a quoted number for exactly this reason. See [Trace Tail Sampling](../banyandb/tail-sampling.md) for how a trace is judged, and [BanyanDB storage](../setup/backend/storages/b
anyandb.md) for the configuration keys.
* Fix: a blank value in `bydb.yml` (`key:` with nothing after it) aborted OAP startup with an opaque `NullPointerException` from `java.util.Properties`, which rejects null values. The BanyanDB config loader now skips blank entries and leaves the field at its default, the same outcome as omitting the line.
* Route LAL rules within a layer by their input type, so a single layer can host rules over different proto inputs. Each compiled rule now carries its effective input type (the proto class its `parsed.*` getters cast to, or `null` for parser-based / untyped rules), and `LogFilterListener` skips any rule whose type doesn't match the incoming log instead of running every rule in the layer. This fixes a latent `ClassCastException` (caught and logged per log) that fired whenever a `MESH` log of one shape reached a rule compiled for another — e.g. an Envoy TCP access log or a network-profiling `LogData` hitting the HTTP `envoy-als` rule. Adds an `envoy-als-tcp` rule (`inputType: TCPAccessLogEntry`) alongside the existing HTTP `envoy-als`; both share the `MESH` layer and each now only sees its own entry type.
+* Fix the PagerDuty alarm hook posting to the Events API v2 endpoint defaults to `https://events.pagerduty.com/v2/enqueue`.
Review Comment:
This changelog line is grammatically unclear (`Fix ... posting ... defaults to ...`). Reword to something like “Fix PagerDuty alarm hook to default the Events API v2 endpoint to ...” or “Fix PagerDuty alarm hook default Events API v2 endpoint to ...” to make the change unambiguous for readers.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make the PagerDuty Events API v2 endpoint configurable. [skywalking]
Posted by "wankai123 (via GitHub)" <gi...@apache.org>.
wankai123 commented on code in PR #13968:
URL: https://github.com/apache/skywalking/pull/13968#discussion_r3719061874
##########
oap-server/server-alarm-plugin/src/test/java/org/apache/skywalking/oap/server/core/alarm/provider/pagerduty/PagerDutyHookCallbackTest.java:
##########
@@ -24,14 +24,88 @@
import org.apache.skywalking.oap.server.core.alarm.provider.AlarmRulesWatcher;
import org.apache.skywalking.oap.server.core.alarm.provider.Rules;
import org.apache.skywalking.oap.server.core.source.DefaultScopeDefine;
+import com.google.gson.Gson;
+import com.google.gson.JsonObject;
+import com.linecorp.armeria.common.HttpResponse;
+import com.linecorp.armeria.common.HttpStatus;
+import com.linecorp.armeria.server.ServerBuilder;
+import com.linecorp.armeria.testing.junit5.server.ServerExtension;
+import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Disabled;
import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.RegisterExtension;
import java.util.Arrays;
import java.util.List;
+import java.util.concurrent.CopyOnWriteArrayList;
public class PagerDutyHookCallbackTest {
+ private static final List<JsonObject> RECEIVED = new CopyOnWriteArrayList<>();
+
+ @RegisterExtension
+ public static final ServerExtension SERVER = new ServerExtension() {
+ @Override
+ protected void configure(ServerBuilder sb) {
+ sb.service("/v2/enqueue", (ctx, req) -> HttpResponse.from(
+ req.aggregate().thenApply(r -> {
+ RECEIVED.add(new Gson().fromJson(r.contentUtf8(), JsonObject.class));
+ return HttpResponse.of(HttpStatus.OK);
+ })
+ ));
+ }
+ };
+
+ /**
+ * The endpoint shipped as {@code http://} from 9.2.0 until this guard was added, putting the integration key —
+ * which {@code getMessageBody} places in the request body — in the clear on every alarm.
+ */
+ @Test
+ public void defaultEndpointMustUseTls() {
+ Assertions.assertTrue(
+ PagerDutySettings.DEFAULT_EVENTS_API_URL.startsWith("https://"),
+ "PagerDuty endpoint must use https, the routing key is sent in the request body, but was: "
+ + PagerDutySettings.DEFAULT_EVENTS_API_URL
+ );
+ PagerDutySettings settings = new PagerDutySettings("s", AlarmHooksType.pagerduty, true);
+ Assertions.assertEquals(
+ PagerDutySettings.DEFAULT_EVENTS_API_URL, settings.getEventsApiUrl(),
+ "a hook that does not configure events-api-url must fall back to the TLS default"
+ );
+ }
+
+ @Test
+ public void testDoAlarmPostsToConfiguredEndpoint() throws Exception {
+ RECEIVED.clear();
+ Rules rules = new Rules();
+ PagerDutySettings setting = new PagerDutySettings("setting1", AlarmHooksType.pagerduty, true);
+ setting.setEventsApiUrl("http://127.0.0.1:" + SERVER.httpPort() + "/v2/enqueue");
+ setting.setIntegrationKeys(Arrays.asList("key-1", "key-2"));
+ setting.setTextTemplate("Apache SkyWalking Alarm: \\n %s.");
+ rules.getPagerDutySettingsMap().put(setting.getFormattedName(), setting);
+
+ AlarmMessage alarmMessage = new AlarmMessage();
+ alarmMessage.setScopeId(DefaultScopeDefine.SERVICE);
+ alarmMessage.setRuleName("service_resp_time_rule");
+ alarmMessage.setAlarmMessage("alarmMessage with [DefaultScopeDefine.All]");
+ alarmMessage.getHooks().add(setting.getFormattedName());
+
+ new PagerDutyHookCallback(new AlarmRulesWatcher(rules, null, null))
+ .doAlarm(new ArrayList<>(List.of(alarmMessage)));
Review Comment:
wrong report
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org