You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by "rstpv (via GitHub)" <gi...@apache.org> on 2024/10/30 15:10:50 UTC
[PR] [FLINK-30284][flink-metrics] Adding datacenter url property to datadog [flink]
rstpv opened a new pull request, #25592:
URL: https://github.com/apache/flink/pull/25592
## What is the purpose of the change
This change deprecates the `dataCenter` configuration for DataDog reporter and replaces it with `dataCenterUrl` that allows users to configure whole address of the data center to use.
## Brief change log
- Deprecates dataCenter key from DataDog reporter configuration (logs a warning to the users).
- Add new configuration option dataCenterUrl that accepts full address of Data Dog site, it defaults to US data center.
## Verifying this change
Please make sure both new and modified tests in this PR follow [the conventions for tests defined in our code quality guide](https://flink.apache.org/how-to-contribute/code-style-and-quality-common/#7-testing).
This change is already covered by existing tests, such as `DatadogHttpClientTest`
## Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): no
- The public API, i.e., is any changed class annotated with `@Public(Evolving)`: no
- The serializers: (yes / no / don't know): no
- The runtime per-record code paths (performance sensitive): (yes / no / don't know): no
- Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: (yes / no / don't know): no
- The S3 file system connector: (yes / no / don't know): no
## Documentation
- Does this pull request introduce a new feature? (yes / no): yes
- If yes, how is the feature documented? docs
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-30284][flink-metrics] Adding datacenter url property to datadog [flink]
Posted by "flinkbot (via GitHub)" <gi...@apache.org>.
flinkbot commented on PR #25592:
URL: https://github.com/apache/flink/pull/25592#issuecomment-2447488993
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "9a14a7c212b387a58ba23f5ffde45d21a2c8a57c",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "9a14a7c212b387a58ba23f5ffde45d21a2c8a57c",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 9a14a7c212b387a58ba23f5ffde45d21a2c8a57c UNKNOWN
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run azure` re-run the last Azure build
</details>
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-30284][flink-metrics] Adding datacenter url property to datadog [flink]
Posted by "davidradl (via GitHub)" <gi...@apache.org>.
davidradl commented on code in PR #25592:
URL: https://github.com/apache/flink/pull/25592#discussion_r1878586368
##########
flink-metrics/flink-metrics-datadog/src/main/java/org/apache/flink/metrics/datadog/DatadogHttpClient.java:
##########
@@ -60,7 +60,7 @@ public DatadogHttpClient(
String dgApiKey,
String dgProxyHost,
int dgProxyPort,
- DataCenter dataCenter,
+ String dataCenterUrl,
Review Comment:
I think we should add a new constructor and deprecate the old one , for backwards compatibility.
These methods should all be tagged with @publicEvolving or @Internal as appropriate.
We should add @deprecate to DataCenter also.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-30284][flink-metrics] Adding datacenter url property to datadog [flink]
Posted by "pedromazala (via GitHub)" <gi...@apache.org>.
pedromazala commented on code in PR #25592:
URL: https://github.com/apache/flink/pull/25592#discussion_r2006170024
##########
flink-metrics/flink-metrics-datadog/src/main/java/org/apache/flink/metrics/datadog/DatadogHttpReporterFactory.java:
##########
@@ -44,10 +46,18 @@ public MetricReporter createMetricReporter(Properties config) {
final String apiKey = config.getProperty(API_KEY, null);
final String proxyHost = config.getProperty(PROXY_HOST, null);
final int proxyPort = Integer.valueOf(config.getProperty(PROXY_PORT, "8080"));
+
+ if (config.containsKey(DATA_CENTER)) {
+ LOG.warn(
+ "The 'dataCenter' option is deprecated; please use 'dataCenterUrl' instead.");
Review Comment:
```suggestion
"The '{}' option is deprecated; please use '{}' instead.", DATA_CENTER, DATA_CENTER_URL);
```
##########
flink-metrics/flink-metrics-datadog/src/main/java/org/apache/flink/metrics/datadog/DatadogHttpReporterFactory.java:
##########
@@ -44,10 +46,18 @@ public MetricReporter createMetricReporter(Properties config) {
final String apiKey = config.getProperty(API_KEY, null);
final String proxyHost = config.getProperty(PROXY_HOST, null);
final int proxyPort = Integer.valueOf(config.getProperty(PROXY_PORT, "8080"));
+
+ if (config.containsKey(DATA_CENTER)) {
+ LOG.warn(
+ "The 'dataCenter' option is deprecated; please use 'dataCenterUrl' instead.");
+ }
final String rawDataCenter = config.getProperty(DATA_CENTER, "US");
+ final String dataCenterUrl = config.getProperty(
+ DATA_CENTER_URL,
+ String.format(DEFAULT_DATACENTER_DOMAIN, DataCenter.valueOf(rawDataCenter).getDomain()));
Review Comment:
And then have something like `DatadogHttpClient.getDefaultDomain(dataCenter)`
##########
flink-metrics/flink-metrics-datadog/src/main/java/org/apache/flink/metrics/datadog/DatadogHttpReporterFactory.java:
##########
@@ -30,11 +30,13 @@
public class DatadogHttpReporterFactory implements MetricReporterFactory {
private static final Logger LOG = LoggerFactory.getLogger(DatadogHttpReporterFactory.class);
+ private static final String DEFAULT_DATACENTER_DOMAIN = "https://app.datadoghq.%s";
Review Comment:
Perhaps we could make all HTTP-related data live on `DatadogHttpClient`? This way we centralize HTTP knowledge in a single place.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-30284][flink-metrics] Adding datacenter url property to datadog [flink]
Posted by "davidradl (via GitHub)" <gi...@apache.org>.
davidradl commented on code in PR #25592:
URL: https://github.com/apache/flink/pull/25592#discussion_r1878598521
##########
flink-metrics/flink-metrics-datadog/src/main/java/org/apache/flink/metrics/datadog/DatadogHttpReporter.java:
##########
@@ -67,20 +67,20 @@ public DatadogHttpReporter(
String proxyHost,
int proxyPort,
int maxMetricsPerRequestValue,
- DataCenter dataCenter,
+ String dataCenterUrl,
String tags,
boolean useLogicalIdentifier) {
this.maxMetricsPerRequestValue = maxMetricsPerRequestValue;
this.useLogicalIdentifier = useLogicalIdentifier;
- this.client = new DatadogHttpClient(apiKey, proxyHost, proxyPort, dataCenter, true);
+ this.client = new DatadogHttpClient(apiKey, proxyHost, proxyPort, dataCenterUrl, true);
this.configTags = getTagsFromConfig(tags);
LOGGER.info(
- "Configured DatadogHttpReporter with {tags={}, proxyHost={}, proxyPort={}, dataCenter={}, maxMetricsPerRequest={}",
+ "Configured DatadogHttpReporter with {tags={}, proxyHost={}, proxyPort={}, dataCenterUrl={}, maxMetricsPerRequest={}",
Review Comment:
I was thinking you would keep the original constructor and deprecate it and add a new constructor for the url.
The log here would then come out reflecting the users config.
I see you have refactored the callers to send through the url. deprecating might be cleaner. WDYT ?
The methods should all have @publicEvolving @internal tags as appropriate in all the methods in all the classes
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-30284][flink-metrics] Adding datacenter url property to datadog [flink]
Posted by "davidradl (via GitHub)" <gi...@apache.org>.
davidradl commented on code in PR #25592:
URL: https://github.com/apache/flink/pull/25592#discussion_r1878603816
##########
docs/content/docs/deployment/metric_reporters.md:
##########
@@ -257,7 +257,8 @@ Parameters:
- `apikey` - the Datadog API key
- `proxyHost` - (optional) The proxy host to use when sending to Datadog.
- `proxyPort` - (optional) The proxy port to use when sending to Datadog, defaults to 8080.
-- `dataCenter` - (optional) The data center (`EU`/`US`) to connect to, defaults to `US`.
+- `dataCenter` - (deprecated/optional) The data center (`EU`/`US`) to connect to, defaults to `US`. *DEPRECATED* will be removed in a future flink version, please use `dataCenterUrl`.
+- `dataCenterUrl`- (optional) The datacenter url to connect to, defaults to `https://app.datadoghq.com`.
Review Comment:
I suggest removing the deprecated option from the example below.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-30284][flink-metrics] Adding datacenter url property to datadog [flink]
Posted by "davidradl (via GitHub)" <gi...@apache.org>.
davidradl commented on code in PR #25592:
URL: https://github.com/apache/flink/pull/25592#discussion_r1878586368
##########
flink-metrics/flink-metrics-datadog/src/main/java/org/apache/flink/metrics/datadog/DatadogHttpClient.java:
##########
@@ -60,7 +60,7 @@ public DatadogHttpClient(
String dgApiKey,
String dgProxyHost,
int dgProxyPort,
- DataCenter dataCenter,
+ String dataCenterUrl,
Review Comment:
I think we should add a new constructor and deprecate the old one , for backwards compatibility.
These methods should all be tagged with @publicEvolving or @Internal as appropriate.
We should add @deprecate to DataCenter also.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-30284][flink-metrics] Adding datacenter url property to datadog [flink]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #25592:
URL: https://github.com/apache/flink/pull/25592#issuecomment-2986786606
This PR is being marked as stale since it has not had any activity in the last 90 days.
If you would like to keep this PR alive, please leave a comment asking for a review.
If the PR has merge conflicts, update it with the latest from the base branch.
If you are having difficulty finding a reviewer, please reach out to the
community, contact details can be found here: https://flink.apache.org/what-is-flink/community/
If this PR is no longer valid or desired, please feel free to close it.
If no activity occurs in the next 30 days, it will be automatically closed.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org