Skip to content
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

Ability to add custom fields to GelfMessage, computed from ILoggingEvent #55

Closed
nemecec opened this issue Nov 25, 2020 · 20 comments
Closed
Milestone

Comments

@nemecec
Copy link

nemecec commented Nov 25, 2020

I would like to implement error grouping similar to Bugsnag. For this, I would like to add a group identifier as a custom field on each relevant GELF message (computed based on raw message, some stack frames, etc).
Currently, this is not easy, so I would like to propose some sort of extension point to make it easier. A kind of "enrichment" processors.

Basically, it would boil down to turning GelfEncoder.mapAdditionalFields(ILoggingEvent event) method into an interface. I'll make a PR to exemplify this.

If you want to explore this idea further, you could even refactor GelfEncoder and replace all include* properties with specific built-in "enrichment" processors. This would simplify that class A LOT.

Usage sample:

<configuration>
  <appender name="GELF" class="de.siegmar.logbackgelf.GelfTcpAppender">
        <graylogHost>localhost</graylogHost>
        <graylogPort>12201</graylogPort>
        <connectTimeout>15000</connectTimeout>
        <reconnectInterval>300</reconnectInterval>
        <maxRetries>2</maxRetries>
        <retryDelay>3000</retryDelay>
        <poolSize>2</poolSize>
        <poolMaxWaitTime>5000</poolMaxWaitTime>
        <poolMaxIdleTime>10</poolMaxIdleTime>
        <encoder class="de.siegmar.logbackgelf.GelfEncoder">
            <originHost>localhost</originHost>
            <additionalFieldMapper class="my.package.GelfGroupIdentifierGenerator"/>
        </encoder>
  </appender>
</configuration>
nemecec pushed a commit to nemecec/logback-gelf that referenced this issue Nov 25, 2020
@osiegmar
Copy link
Owner

Thanks for your ideas! I think, a more generic approach to map the additional fields (as you described) would be very good and makes it easier extensible. Would be great if you could provide a PR.

@nemecec
Copy link
Author

nemecec commented Nov 29, 2020

I added a PR together with the issue, you didn't notice it?

@osiegmar
Copy link
Owner

I noticed, but more things needs to be done, like test, documentation, examples, ...

@nemecec
Copy link
Author

nemecec commented Nov 29, 2020

True, I did not want to invest too much time without first getting a confirmation from you that the direction is acceptable.

@nemecec
Copy link
Author

nemecec commented Dec 15, 2020

Is the direction acceptable?

@osiegmar
Copy link
Owner

Sorry for the misunderstanding. You wrote:

If you want to explore this idea further, you could even refactor GelfEncoder and replace all include* properties with specific built-in "enrichment" processors. This would simplify that class A LOT.

With my initial answer I tried to say that I'd like to go that direction. Unfortunately I currently do not have the time to refactor the GelfEncoder by myself so I'd appreciate if you could provide a PR for this.

@nemecec
Copy link
Author

nemecec commented Dec 16, 2020

Great! I plan to do 2 things during the upcoming weeks:

  • add unit tests to the existing PR
  • add another PR that would refactor the GelfEncoder class

@osiegmar
Copy link
Owner

I think it would be the best if you focus on the PR for refactoring the GelfEncoder class right away. I don't want to merge the 'small' solution from the first PR.

@nemecec
Copy link
Author

nemecec commented Jan 11, 2021

I now updated the PR and refactored GelfEncoder as we agreed.
There are some CheckStyle errors:

Error: eckstyle] [ERROR] /home/runner/work/logback-gelf/logback-gelf/src/main/java/de/siegmar/logbackgelf/GelfEncoder.java:43:1: Class Data Abstraction Coupling is 8 (max allowed is 7) classes [CallerDataFieldMapper, GelfFieldHelper, GelfMessage, MarkerFieldMapper, MdcDataFieldMapper, PatternLayout, RootExceptionDataFieldMapper, SimpleFieldMapper]. [ClassDataAbstractionCoupling]

Error: eckstyle] [ERROR] /home/runner/work/logback-gelf/logback-gelf/src/main/java/de/siegmar/logbackgelf/GelfEncoder.java:347: Line is longer than 100 characters (found 102). [LineLength]
> Task :checkstyleMain
Error: eckstyle] [ERROR] /home/runner/work/logback-gelf/logback-gelf/src/main/java/de/siegmar/logbackgelf/GelfFieldHelper.java:81: Line is longer than 100 characters (found 107). [LineLength]
Error: eckstyle] [ERROR] /home/runner/work/logback-gelf/logback-gelf/src/main/java/de/siegmar/logbackgelf/GelfFieldHelper.java:84: Line is longer than 100 characters (found 109). [LineLength]
Error: eckstyle] [ERROR] /home/runner/work/logback-gelf/logback-gelf/src/main/java/de/siegmar/logbackgelf/GelfFieldMapper.java:27: Line is longer than 100 characters (found 111). [LineLength]
Error: eckstyle] [ERROR] /home/runner/work/logback-gelf/logback-gelf/src/main/java/de/siegmar/logbackgelf/mappers/MdcDataFieldMapper.java:46: Line is longer than 100 characters (found 107). [LineLength]

I find these to be rather restrictive and would suggest to lift these:

  • Either remove Class Data Abstraction Coupling check or increase it to at least 10.
  • I would suggest to increase line length limit to allow 120 characters on single line. Monitors are quite large nowadays.

@osiegmar
Copy link
Owner

OK, could you please:

  • Add @SuppressWarnings("checkstyle:classdataabstractioncouling") to the GelfEncoder class in order to let the pipeline complete
  • Change the line length limit to 120 in checkstyle.xml
  • Merge the master (I added code codecov code coverage analysis)

nemecec pushed a commit to nemecec/logback-gelf that referenced this issue Jan 12, 2021
@nemecec
Copy link
Author

nemecec commented Jan 12, 2021

Done

@osiegmar
Copy link
Owner

Thank you. Give me a few days (probably until end of week) to review it.

osiegmar pushed a commit that referenced this issue Jan 16, 2021
* Add additional-field-mapper extension point as described in #55
@osiegmar
Copy link
Owner

Thanks for your work. It really looks great! I'll play a little with it and will perform some small changes. I'll get back to you in order to check if those changes are breaking anything you had in mind.

@osiegmar
Copy link
Owner

You now may want to take a look on c372fe8

@nemecec
Copy link
Author

nemecec commented Jan 17, 2021

Two small improvements to your changes, see pull request #57.

@osiegmar osiegmar added this to the 3.0.0 milestone Jan 18, 2021
@osiegmar
Copy link
Owner

Thanks again!

@nemecec
Copy link
Author

nemecec commented Jan 18, 2021

Any plans for a release?

@osiegmar
Copy link
Owner

@nemecec
Copy link
Author

nemecec commented Sep 3, 2021

Still no release :-( Any plans?

@osiegmar
Copy link
Owner

osiegmar commented Sep 4, 2021

Yep. I also makes me a little sad. In case you have some spare time left, you could help on #59 to finish the release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants