Skip to content

Conversation

RichardLindhout
Copy link

No description provided.

@cla-checker-service
Copy link

cla-checker-service bot commented Mar 8, 2023

💚 CLA has been signed

@RichardLindhout
Copy link
Author

❌ Author of the following commits did not sign a Contributor Agreement: 2e453d3

Please, read and sign the above mentioned agreement if you want to contribute to this project

I did now

@beniwohli
Copy link

Hi @RichardLindhout

Thanks for your PR! Could you explain why you propose this change? It could be considered a backwards incompatible change (as users who already set up dashboards, queries etc. with the lower case level names would have to update them), so we'll need to be very careful with changing this behaviour.

@basepi
Copy link
Contributor

basepi commented Mar 8, 2023

I assume this is so it matches the stdlib logging levels. But I agree with @beniwohli, this is backwards incompatible and while matching logging would be nice I don't think it's worth the churn.

@untergeek
Copy link
Member

I agree with @basepi. It may not match stdlib, but part of the point of using Elasticsearch for logging is normalizing formats. Switching to uppercase here would break that convention.

@RichardLindhout
Copy link
Author

It's because in the docs of elasticsearch + other services of us log in UPPERCASE. However I managed to get it uppercase by using a custom log formatter

class CustomFormatter(ecs_logging.StdlibFormatter):
    def format_to_ecs(self, record):
        result = super().format_to_ecs(record)
        result["log"]['level'] = result["log"]['level'].upper()
        return result

@RichardLindhout
Copy link
Author

E.g.:https://www.elastic.co/guide/en/elasticsearch/reference/current/logging.html#configuring-logging-levels But maybe everything in the docs is written for Java, so we can probably close this PR.

@basepi basepi closed this Mar 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants