From fa1e4bd74df6937cb9e5091717b98bc2ff619736 Mon Sep 17 00:00:00 2001 From: Arya <90748009+aryamohanan@users.noreply.github.com> Date: Mon, 9 Sep 2024 09:47:02 +0530 Subject: [PATCH] fix: added deprecation warning for Kafka header migration (#1311) - Introduced a deprecation warning for Kafka header migration phase-2. - In the next major release (v4), the configuration option for Kafka header format will be removed. - The default Kafka header format will be set to 'string'. - refs INSTA-809 --- packages/aws-fargate/test/using_api/test.js | 7 ++++- .../instrumentation/messaging/kafkaJs.js | 13 ++++++++++ .../instrumentation/messaging/rdkafka.js | 26 +++++++------------ .../google-cloud-run/test/using_api/test.js | 8 +++++- 4 files changed, 36 insertions(+), 18 deletions(-) diff --git a/packages/aws-fargate/test/using_api/test.js b/packages/aws-fargate/test/using_api/test.js index 83be97fc3..f917b26d1 100644 --- a/packages/aws-fargate/test/using_api/test.js +++ b/packages/aws-fargate/test/using_api/test.js @@ -110,11 +110,16 @@ describe('Using the API', function () { // During phase 1 of the Kafka header migration (October 2022 - October 2023) there will be a debug log about // ignoring the option 'both' for rdkafka. We do not care about that log message in this test. const debug = response.logs.debug.filter(msg => !msg.includes('Ignoring configuration or default value')); + + // As part of the Kafka header migration phase 2, we have added warning logs regarding the removal of the option + // to configure Kafka header formats. This test skips the warning message, and the warning itself will be removed + // in the next major release. + const warn = response.logs.warn.filter(msg => !msg.includes('Kafka header format')); expect(debug).to.contain('Sending data to Instana (/serverless/metrics).'); expect(debug).to.contain('Sent data to Instana (/serverless/metrics).'); expect(response.logs.info).to.be.empty; - expect(response.logs.warn).to.deep.equal([ + expect(warn).to.deep.equal([ 'INSTANA_DISABLE_CA_CHECK is set, which means that the server certificate will not be verified against the ' + 'list of known CAs. This makes your service vulnerable to MITM attacks when connecting to Instana. This ' + 'setting should never be used in production, unless you use our on-premises product and are unable to ' + diff --git a/packages/core/src/tracing/instrumentation/messaging/kafkaJs.js b/packages/core/src/tracing/instrumentation/messaging/kafkaJs.js index d511b4c9b..26973ba9a 100644 --- a/packages/core/src/tracing/instrumentation/messaging/kafkaJs.js +++ b/packages/core/src/tracing/instrumentation/messaging/kafkaJs.js @@ -43,6 +43,9 @@ exports.activate = function activate(extraConfig) { headerFormat = extraConfig.tracing.kafka.headerFormat; } } + if (headerFormat) { + logWarningForKafkaHeaderFormat(); + } isActive = true; }; @@ -532,3 +535,13 @@ function removeInstanaHeadersFromMessage(message) { }); } } + +// Note: This function can be removed as soon as we finish the Kafka header migration phase2. +// Might happen in major release v4. +function logWarningForKafkaHeaderFormat() { + logger.warn( + '[Deprecation Warning] The configuration option for specifying the Kafka header format will be removed in the ' + + 'next major release as the format will no longer be configurable and Instana tracers will only send string ' + + 'headers. More details see: https://ibm.biz/kafka-trace-correlation-header.' + ); +} diff --git a/packages/core/src/tracing/instrumentation/messaging/rdkafka.js b/packages/core/src/tracing/instrumentation/messaging/rdkafka.js index 682cbd8b1..913ae934f 100644 --- a/packages/core/src/tracing/instrumentation/messaging/rdkafka.js +++ b/packages/core/src/tracing/instrumentation/messaging/rdkafka.js @@ -52,9 +52,14 @@ exports.deactivate = function deactivate() { isActive = false; }; -// Note: This function can be removed as soon as we finish the Kafka header migration and remove the ability to -// configure the header format (at that point, we will only be using string headers). +// Note: This function can be removed as soon as we finish the Kafka header migration phase 2 and remove the ability to +// configure the header format. Might happen in major release v4. function logWarningForKafkaHeaderFormat(headerFormat) { + logger.warn( + '[Deprecation Warning] The configuration option for specifying the Kafka header format will be removed in the ' + + 'next major release as the format will no longer be configurable and Instana tracers will only send string ' + + 'headers. More details see: https://ibm.biz/kafka-trace-correlation-header.' + ); // node-rdkafka's handling of non-string header values is broken, see // https://github.com/Blizzard/node-rdkafka/pull/968. // @@ -65,21 +70,10 @@ function logWarningForKafkaHeaderFormat(headerFormat) { // Trace correlation would be broken for rdkafka senders with the header format 'binary'. If that format has been // configured explicitly, we log a warning and ignore the config value. The rdkafka instrumentation alwas acts as if // format 'string' had been configured. - if (headerFormat === 'binary') { + if (headerFormat === 'binary' || headerFormat === 'both') { logger.warn( - "Ignoring configuration value 'binary' for Kafka header format in node-rdkafka instrumentation, using header " + - "format 'string' instead. Binary headers do not work with node-rdkafka, see " + - 'https://github.com/Blizzard/node-rdkafka/pull/968.' - ); - } else if (headerFormat === 'both') { - // The option format 'both' which is available for other tracers/instrumentations (sending both binary and string - // headers also does not make sense for node-rdkafka headers, because sending binary headers along with string - // headers will not have any benefit. Theoretically, we would also want to warn if 'both' has been configured - // explicitly. But both is also the current default value and we cannot differentiate between an explicit - // configuration and the default value here, so we do not log a warning for 'both', just a debug message. - logger.debug( - "Ignoring configuration or default value 'both' for Kafka header format in node-rdkafka instrumentation, using " + - "header format 'string' instead. Binary headers do not work with node-rdkafka, see " + + `Ignoring configuration value '${headerFormat}' for Kafka header format in node-rdkafka instrumentation, ` + + " using header format 'string' instead. Binary headers do not work with node-rdkafka, see " + 'https://github.com/Blizzard/node-rdkafka/pull/968.' ); } diff --git a/packages/google-cloud-run/test/using_api/test.js b/packages/google-cloud-run/test/using_api/test.js index d454a8c61..c230cca5d 100644 --- a/packages/google-cloud-run/test/using_api/test.js +++ b/packages/google-cloud-run/test/using_api/test.js @@ -104,13 +104,19 @@ describe('Using the API', function () { // During phase 1 of the Kafka header migration (October 2022 - October 2023) there will be a debug log about // ignoring the option 'both' for rdkafka. We do not care about that log message in this test. + const debug = response.logs.debug.filter(msg => !msg.includes('Ignoring configuration or default value')); + // As part of the Kafka header migration phase 2, we have added warning logs regarding the removal of the option to + // configure Kafka header formats. This test skips the warning message, and the warning itself will be removed + // in the next major release. + const warn = response.logs.warn.filter(msg => !msg.includes('Kafka header format')); + expect(debug).to.contain('Sending data to Instana (/serverless/metrics).'); expect(debug).to.contain('Sent data to Instana (/serverless/metrics).'); expect(response.logs.info).to.be.empty; - expect(response.logs.warn).to.deep.equal([ + expect(warn).to.deep.equal([ 'INSTANA_DISABLE_CA_CHECK is set, which means that the server certificate will not be verified against the ' + 'list of known CAs. This makes your service vulnerable to MITM attacks when connecting to Instana. This ' + 'setting should never be used in production, unless you use our on-premises product and are unable to ' +