Skip to content

Exception while exporting logs: Invalid type {type(value)} of value {value} #3389

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
rayrapetyan opened this issue Jul 22, 2023 · 14 comments
Closed
Labels
bug Something isn't working

Comments

@rayrapetyan
Copy link

Steps to reproduce

try:
   ...
except Exception as e:
   logging.warning(e)

What is the expected behavior?
No "Exception while exporting logs" errors in log

What is the actual behavior?
"Exception while exporting logs" error in log

Additional info

{"body":"Exception while exporting logs.","severity":"ERROR","attributes":{"exception.message":"Invalid type \u003cclass 'ValueError'\u003e of value invalid literal for int() with base 10: ''","exception.stacktrace":"Traceback (most recent call last):
  File \"/usr/local/lib/python3.9/site-packages/opentelemetry/sdk/_logs/_internal/export/__init__.py\", line 312, in _export_batch
    self._exporter.export(self._log_records[:idx])  # type: ignore
  File \"/usr/local/lib/python3.9/site-packages/opentelemetry/exporter/otlp/proto/grpc/_log_exporter/__init__.py\", line 108, in export
    return self._export(batch)
  File \"/usr/local/lib/python3.9/site-packages/opentelemetry/exporter/otlp/proto/grpc/exporter.py\", line 276, in _export
    request=self._translate_data(data),
  File \"/usr/local/lib/python3.9/site-packages/opentelemetry/exporter/otlp/proto/grpc/_log_exporter/__init__.py\", line 105, in _translate_data
    return encode_logs(data)
  File \"/usr/local/lib/python3.9/site-packages/opentelemetry/exporter/otlp/proto/common/_internal/_log_encoder/__init__.py\", line 38, in encode_logs
    return ExportLogsServiceRequest(resource_logs=_encode_resource_logs(batch))
  File \"/usr/local/lib/python3.9/site-packages/opentelemetry/exporter/otlp/proto/common/_internal/_log_encoder/__init__.py\", line 61, in _encode_resource_logs
    pb2_log = _encode_log(sdk_log)
  File \"/usr/local/lib/python3.9/site-packages/opentelemetry/exporter/otlp/proto/common/_internal/_log_encoder/__init__.py\", line 47, in _encode_log
    body=_encode_value(log_data.log_record.body),
  File \"/usr/local/lib/python3.9/site-packages/opentelemetry/exporter/otlp/proto/common/_internal/__init__.py\", line 79, in _encode_value
    raise Exception(f\"Invalid type {type(value)} of value {value}\")
Exception: Invalid type \u003cclass 'ValueError'\u003e of value invalid literal for int() with base 10: ''
","exception.type":"Exception"},"resources":{"telemetry.sdk.language":"python","telemetry.sdk.name":"opentelemetry","telemetry.sdk.version":"1.19.0"},"instrumentation_scope":{"name":"opentelemetry.sdk._logs._internal"}}

I think _encode_value at

should first try to convert value to string explicitly and only raise if conversion fails.

@rayrapetyan rayrapetyan added the bug Something isn't working label Jul 22, 2023
@gangeshwark
Copy link

I have had the same issue since updating to Version 1.19.0/0.40b0

@pmcollins
Copy link
Member

Thanks for the submission. Do either of you have a minimal program or test to reproduce this?

@ocelotl
Copy link
Contributor

ocelotl commented Aug 8, 2023

Not sure if we should be making any changes here, the supported types are defined in the proto.

@lzchen
Copy link
Contributor

lzchen commented Sep 25, 2023

@rayrapetyan @gangeshwark

Ping to have a repro of this issue. There should not be other value types besides the ones defined in the proto.

@ocelotl
Copy link
Contributor

ocelotl commented Oct 3, 2023

@lzchen are you ok with closing this issue?

@lzchen lzchen closed this as completed Oct 3, 2023
@lzchen
Copy link
Contributor

lzchen commented Oct 3, 2023

Feel free to comment here if want to reopen.

@siminn-arnorgj
Copy link

I see logging of raw objects somewhat frequently in Python codebases. @lzchen @ocelotl Would you be opposed to adding a default argument to OTLPLogExporter similar to the argument passed into json.dumps?
Or alternatively make efforts to make _encode_value a method on a subclassable class that could then be injected?

@alexmarshalldw
Copy link

@lzchen @ocelotl I'd like to see this issue reopened. I'm able to reproduce it. I think @siminn-arnorgj is proposing some valid options that deserve consideration, and this issue is blocking me from being able to adopt OpenTelemetry Exporters in my Python code. e.g. I'm using this library within the scope of a Django (not my choice) application.

@edinpeter
Copy link

edinpeter commented Jan 4, 2024

I don't have a complete working repro, but the gist of the issue was a code snippet like this:

except fastapi.exceptions.HTTPException as exc:
    logger.error(exc, exc_info=True)
    raise exc

This was raised in otel and prevented our logs from exporting properly, raising the following:
Exception: Invalid type <class 'fastapi.exceptions.HTTPException'> of value

Fix was just changing logger.error(exc, exc_info=True) -> logger.error(exc.detail, exc_info=True) in compliance with the type restrictions linked in the proto above

@tim-win
Copy link

tim-win commented Jul 28, 2024

I'd also like this issue re-opened. While I'm happy to avoid passing custom types to the logger, it becomes very noisy when third party libraries do this, and the twisted python library that daphne is based on thinks this is an acceptable practice. As the python logger interface supports coercion via str or repr, the least surprising thing would be to do the same here.

@tim-win
Copy link

tim-win commented Jul 28, 2024

For those who follow and are using daphne like I am, the easiest way to deal with this is to patch via a super brittle diff file that will break on any bump to opentelemetry versions:

88c88,92
<     raise Exception(f"Invalid type {type(value)} of value {value}")
---
>     try:
>         return PB2AnyValue(string_value=str(value))
>     except:
>         pass
>     raise Exception(f"Invalid type {type(value)} of value {value}")

I'm using this on opentelemetry-exporter-otlp==1.26.0, you'll want to make your own diff with correct line numbers. The alternative to monkeypatch the opentelemetry-instrument command seems like it may be more robust, but the OTLP wrapper already is doing a bit more magic than I'm comfortable with.

I'll also look at the other option, convincing the twisted web team to change their logging practices, and then get daphne to bump their twisted version, and then bump the version of daphne I'm using.

@pmcollins
Copy link
Member

If anyone can supply a reproducing script that would be helpful -- I am unable to reproduce this myself.

@siminn-arnorgj
Copy link

siminn-arnorgj commented Aug 1, 2024

If anyone can supply a reproducing script that would be helpful -- I am unable to reproduce this myself.

This happens every time an object that is not a bool, str, int, float, Sequence, or Mapping is logged directly or sits within one of those structures. In my experience, the most common way this occurs is when exceptions are logged directly or when libraries implement their own type for logging, such as pymongo's pymongo.logger.LogMessage or twisted's twisted.logger._stdlib.StringifiableFromEvent.

Here is a simple script to reproduce this issue:

opentelemetry-exporter-otlp==1.26.0
pymongo==4.8.0
twisted==24.3.0
from opentelemetry.sdk._logs.export import BatchLogRecordProcessor
from opentelemetry.sdk._logs import LoggerProvider, LoggingHandler
from opentelemetry.exporter.otlp.proto.grpc._log_exporter import OTLPLogExporter
import logging


def create_logging_handler():
    logger_provider = LoggerProvider()
    logger_provider.add_log_record_processor(
        BatchLogRecordProcessor(OTLPLogExporter(insecure=True))
    )
    otel_handler = LoggingHandler(logger_provider=logger_provider)
    return otel_handler


def main():
    logging.basicConfig(level=logging.NOTSET)
    logging.getLogger().addHandler(
        create_logging_handler()
    )  # Add the handler to the root logger

    logger = logging.getLogger(__name__)

    try:
        raise ValueError("Err")
    except ValueError as e:
        logger.error(e)  # Logging of the raw exception object

    import pymongo

    pymongo.MongoClient("Cause pymongo to emit logs")

    from twisted.logger import Logger, globalLogBeginner, STDLibLogObserver

    globalLogBeginner.beginLoggingTo([STDLibLogObserver()])
    Logger().info("Cause custom log message")


if __name__ == "__main__":
    main()

The most elegant and robust workaround I have found is to subclass the LoggingHandler and modify the record directly before it's emitted like so:

def create_logging_handler():
    from pymongo.logger import LogMessage as pymongoLogMessage
    from twisted.logger._stdlib import StringifiableFromEvent

    logger_provider = LoggerProvider()
    logger_provider.add_log_record_processor(
        BatchLogRecordProcessor(OTLPLogExporter(insecure=True))
    )

    class ModifiedHandler(LoggingHandler):
        def emit(self, record):
            to_str_types = (Exception, pymongoLogMessage, StringifiableFromEvent)
            if isinstance(record.msg, to_str_types):
                record.msg = str(record.message)
            super().emit(record)

    otel_handler = ModifiedHandler(logger_provider=logger_provider)
    return otel_handler

This workaround fails if the unknown type is in a nested structure like [Exception()] but i have never seen this come from a library.

I still feel like there should be a way to influence the exporter's behavior when encountering an unknown type. My suggestion is to either make the encoder an object/class which can be provided to the exporter, like so:
OTLPLogExporter(insecure=True, encoder=ModifiedEncoder())
or simply allow providing a default function in case the encoding fails:
OTLPLogExporter(insecure=True, default_enc=str)

@pmcollins
Copy link
Member

pmcollins commented Aug 5, 2024

Thanks for the excellent repro -- gives some context.

The original issue states:

should first try to convert value to string explicitly and only raise if conversion fails.

This seems like a reasonable idea. It conforms to the types supported by the protobuf spec, will eliminate the otel stacktrace logging mentioned above, and it gives other libraries some control over how their logged objects are represented when logged by otel by changing the __str__ implementation.

Are there counterarguments?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

9 participants