Implement partial success logging in OTLP exporters

[michaelsafyan]

Log partial success

Partial successes during OTLP export is now logged.

Fixes #4803

Type of change

• New feature (non-breaking change which adds functionality)
• [?] This change requires a documentation update

How Has This Been Tested?

Added new unit tests covering the relevant cases.

Does This PR Require a Contrib Repo Change?

• No.

Checklist:

• Followed the style guidelines of this project
• Changelogs have been updated
• Unit tests have been added
• [] Documentation has been updated - unsure what to update

[DylanRussell]

Why put this log behind an env var ? Why not just always log when the field is set -- I'm assuming it's only set when there was a partial success -- it doesn't sound too noisy to me.

[michaelsafyan]

Why put this log behind an env var ? Why not just always log when the field is set -- I'm assuming it's only set when there was a partial success -- it doesn't sound too noisy to me.

I'm uncertain as to the level of noisiness, and so I wanted to minimize disruption to existing users. I could imagine it being somewhat annoying if every batch included one persistently malformed span that just ended up creating log spam.

A more sophisticated approach might be to opt-in to recording every partial success, while elevating the verbosity and logging the partial success unconditionally if every record was a failure (if num_rejected_{type} matches the number of items in the batch). But I wanted to keep things relatively simple.

[DylanRussell]

We already have the DuplicateFilter on this logger, maybe we should tweak that to de-dupe based on line no and module instead of message which can be unique so that it filters stuff out better.

I think it's a bit weird to put just 1 log message behind this env var

[michaelsafyan]

Thanks for the review, @DylanRussell . I updated the logic to no longer make the logging conditioned on OTEL_LOG_LEVEL. I'm not sure what the DuplicateFilter is you are referring to ... are you asking to also remove the special casing for logs (because you are not concerned about partial success causing a loop?)

[DylanRussell]

nit: can we just inline this code instead of having a function (same thing below)

[DylanRussell]

We add a DuplicateFilter to the logger on line 116 (as an aside I don't like the github code review tool, which doesn't let me add a comment to a line that wasn't modified...)

I'm thinking we should update that to reference record.lineno instead of record.msg which could be unique.... and that is sufficient to suppress noisy logging, and also avoid the endless logging issues...

If you want to take a stab at that change here that'd be great, if not I think we can proceed without that and I can make it separately..

I don't think we should special case the logging exporter because we have this duplicate filter thing.

[michaelsafyan]

Thanks, @DylanRussell ! Updated accordingly. Please take another look.

Also, any idea what's going on with the failed tests? They look to be unrelated to this change:

[DylanRussell]

That's some sort of flake we get all the time (can't find a commit hash), IDK why we get that. It's OK to ignore

[DylanRussell]

nit: can we inline this code instead ? IMO it's more readable

[DylanRussell]

IMO I would simplify all this and just have the logic as:

        current_log_by_line = (
            record.module,
            record.pathname,
            record.lineno,
            time_boundary,
        )
        previous_log_by_line = getattr(self, "last_source", None)
        keep_processing = current_log_by_line != previous_log_by_line
        self.last_source = current_log_by_line 
        return keep_processing

Although I'm realizing now that we should store the previous 10 previous_log_by_line in like a deque or OrderedDict instead.. Right now if duplicate log messages are interleaved we will not filter anything out

Also we have tests for this thing here:

opentelemetry-python/opentelemetry-sdk/tests/shared_internal/test_batch_processor.py

Lines 259 to 266 in 62da90e

	class TestCommonFuncs(unittest.TestCase):
	def test_duplicate_logs_filter_works(self):
	test_logger = logging.getLogger("testLogger")
	test_logger.addFilter(DuplicateFilter())
	with self.assertLogs("testLogger") as cm:
	test_logger.info("message")
	test_logger.info("message")
	self.assertEqual(len(cm.output), 1)

[DylanRussell]

nit: can revert this

[DylanRussell]

You could also update the HTTP exporters the same way I think ? https://github.com/open-telemetry/opentelemetry-python/tree/main/exporter/opentelemetry-exporter-otlp-proto-http/src/opentelemetry/exporter/otlp/proto/http