Skip to content

Commit

Permalink
fix: added deprecation warning for Kafka header migration (#1311)
Browse files Browse the repository at this point in the history
- 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
  • Loading branch information
aryamohanan committed Sep 9, 2024
1 parent 283dd73 commit fa1e4bd
Show file tree
Hide file tree
Showing 4 changed files with 36 additions and 18 deletions.
7 changes: 6 additions & 1 deletion packages/aws-fargate/test/using_api/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 ' +
Expand Down
13 changes: 13 additions & 0 deletions packages/core/src/tracing/instrumentation/messaging/kafkaJs.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,9 @@ exports.activate = function activate(extraConfig) {
headerFormat = extraConfig.tracing.kafka.headerFormat;
}
}
if (headerFormat) {
logWarningForKafkaHeaderFormat();
}
isActive = true;
};

Expand Down Expand Up @@ -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.'
);
}
26 changes: 10 additions & 16 deletions packages/core/src/tracing/instrumentation/messaging/rdkafka.js
Original file line number Diff line number Diff line change
Expand Up @@ -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.
//
Expand All @@ -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.'
);
}
Expand Down
8 changes: 7 additions & 1 deletion packages/google-cloud-run/test/using_api/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 ' +
Expand Down

0 comments on commit fa1e4bd

Please sign in to comment.