feat(config): enable telemetry when SCA is on independently from appsec

[florentinl]

What does this PR do?

My understanding is that we want instrumentation telemetry to be off by default in python lambda. There is currently a bypass for AppSec that is a bit weird using env vars directly in __init__.py. I added another bypass for Runtime SCA and moved the logic to the config module to rely on parsed env vars instead.

Motivation

A customer asked if he could enable Runtime SCA without enabling AAP (AppSec) in Lambda. This currently requires setting DD_INSTRUMENTATION_TELEMETRY_ENABLED manually, we could make it a bit more easy.

Testing Guidelines

Additional Notes

• In ddtrace, the telemetry_writer is instantiated and enabled on first import if DD_INSTRUMENTATION_TELEMETRY_ENABLED is not explicitely set to false => The current telemetry enablement code in datadog_lambda/tracing.py is therefore redundant and unnecessary so I removed it.

• The config.enable_telemetry flag was therefore not necessary anymore and I removed it.

• This small refactor relies on the fact that the configuration is resolved before importing ddtrace which happens transitively through cold_start.py. Should we make this more explicit by importing it in __init__.py or is this already clear enough ?

Types of Changes

• Bug fix
• New feature
• Breaking change
• Misc (docs, refactoring, dependency upgrade, etc.)

Check all that apply

• This PR's description is comprehensive
• This PR contains breaking changes that are documented in the description
• This PR introduces new APIs or parameters that are documented and unlikely to change in the foreseeable future
• This PR impacts documentation, and it has been updated (or a ticket has been logged)
• This PR's changes are covered by the automated tests
• This PR collects user input/sensitive content into Datadog
• This PR passes the integration tests (ask a Datadog member to run the tests)

[joeyzhao2018]

In ddtrace, the telemetry_writer is instantiated and enabled on first import if DD_INSTRUMENTATION_TELEMETRY_ENABLED is not explicitely set to false => The current telemetry enablement code in datadog_lambda/tracing.py is therefore redundant and unnecessary so I removed it.

Currently we are turning telemetry off (except for asm case) for both performance concerns and incpompatibilies (many errors). Therefore I believe we need to keep it.

btw, what's Runtime SCA?

[purple4reina]

The reason this code was added here was we must ensure the DD_INSTRUMENTATION_TELEMETRY_ENABLED is set before any ddtrace code is imported. Can we ensure that we the change you're proposing? How can we enforce it?

[florentinl]

The reason this code was added here was we must ensure the DD_INSTRUMENTATION_TELEMETRY_ENABLED is set before any ddtrace code is imported.

I agree. As explained in the PR notes, this is the reason why we need the config to be resolved before the from datadog_lambda.patch import patch_all line of __init__.py.

Can we ensure that we the change you're proposing? How can we enforce it?

We can eagerly import the config in __init__.py and add a test to ensure that importing config.py does not import ddtrace. Would that work for you or does it feel like an unnecessary constraint ? Let me suggest something.

The idea was to reuse the parsing of environment variables done in config if possible. This also looked like some configuration and it felt natural to put it in "config.py".
I can keep it in __init__.py if it make more sense.

[purple4reina]

I think any of those options are fine. As long as we can ensure it's being set before importing ddtrace. I like your idea of adding a test to check for this.

[purple4reina]

We need to ensure that the DD_INSTRUMENTATION_TELEMETRY_ENABLED is set before any ddtrace code is imported.

[florentinl]

Currently we are turning telemetry off (except for asm case) for both performance concerns and incpompatibilies (many errors). Therefore I believe we need to keep it.

Okay wasn't aware of the arguments for keeping it disabled, thank you for the insight. I am definitely not challenging this decision.

btw, what's Runtime SCA?

Runtime Software Composition Analysis (SCA) is a Code Security product that analyzes third party dependencies installed on the application. It relies on instrumentation telemetry to collect the list of dependencies and their versions.