[Essentials] (client) Error Telemetry logging: JEP in early draft review

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

[Essentials] (client) Error Telemetry logging: JEP in early draft review

Baptiste MATHUS-3
Hello everyone,

So as a followup on the recent discussions around telemetry logging for Essentials, I've drafted a JEP on this matter I would love to get feedback on:


Thanks!

-- Baptiste

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANWgJS4bikJoV3vEGdiMbMSzbgAwL1_LMZYKcUZhWBxrcj8qPA%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Essentials] (client) Error Telemetry logging: JEP in early draft review

Jesse Glick-4
Seems reasonable.

The implementation separates JSON records with a newline. This is not
apparent from the JEP; it would be nice to mention it, so that readers
do not wonder whether you are using some kind of streaming JSON parser
(which is unnecessary when each line is a well-formed JSON document).

`JsonFormatter.formatException` can be replaced with
`Functions.printThrowable`, which formats chained exceptions more
readably. The JEP proposal for a future `stacktrace` field neglects to
mention that this will not work as is for exceptions with causes
and/or suppressed exceptions. In general `Throwable.printStackTrace`
can be overridden in nontrivial ways, sometimes including information
not present in the `message` nor stack trace (though less so after
JENKINS-46140), so I would not recommend bothering with anything but a
plain printed string.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANfRfr1mbH2e86BymBfSBcxu4%2BhM0TewBS7XyBreok8AFg9yOA%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Essentials] (client) Error Telemetry logging: JEP in early draft review

Baptiste MATHUS


2018-04-05 0:53 GMT+02:00 Jesse Glick <[hidden email]>:
Seems reasonable.

The implementation separates JSON records with a newline. This is not
apparent from the JEP; it would be nice to mention it, so that readers
do not wonder whether you are using some kind of streaming JSON parser
(which is unnecessary when each line is a well-formed JSON document).

Done.
 

`JsonFormatter.formatException` can be replaced with
`Functions.printThrowable`, which formats chained exceptions more
readably. The JEP proposal for a future `stacktrace` field neglects to
mention that this will not work as is for exceptions with causes
and/or suppressed exceptions. In general `Throwable.printStackTrace`
can be overridden in nontrivial ways, sometimes including information
not present in the `message` nor stack trace (though less so after
JENKINS-46140), so I would not recommend bothering with anything but a
plain printed string.

Ack. Adjusted the proposal too, thanks a lot.

I will submit the PR to official JEP later today I think. Still giving it a few hours for some more feedback but I don't expect much pushback in this area and for such a small JEP anyway.

Cheers

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANWgJS5Dnzki26-42x66E6XcnmxnS3GkKRuxk3r_GuSJ4s7T8g%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Essentials] (client) Error Telemetry logging: JEP in early draft review

Baptiste MATHUS
Submitted as https://github.com/jenkinsci/jep/pull/77

Thanks everyone for the feedback.

2018-04-05 9:38 GMT+02:00 Baptiste Mathus <[hidden email]>:


2018-04-05 0:53 GMT+02:00 Jesse Glick <[hidden email]>:
Seems reasonable.

The implementation separates JSON records with a newline. This is not
apparent from the JEP; it would be nice to mention it, so that readers
do not wonder whether you are using some kind of streaming JSON parser
(which is unnecessary when each line is a well-formed JSON document).

Done.
 

`JsonFormatter.formatException` can be replaced with
`Functions.printThrowable`, which formats chained exceptions more
readably. The JEP proposal for a future `stacktrace` field neglects to
mention that this will not work as is for exceptions with causes
and/or suppressed exceptions. In general `Throwable.printStackTrace`
can be overridden in nontrivial ways, sometimes including information
not present in the `message` nor stack trace (though less so after
JENKINS-46140), so I would not recommend bothering with anything but a
plain printed string.

Ack. Adjusted the proposal too, thanks a lot.

I will submit the PR to official JEP later today I think. Still giving it a few hours for some more feedback but I don't expect much pushback in this area and for such a small JEP anyway.

Cheers

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANWgJS65yv%2BAK31_pvXC8dJafUum2Q_9Ms3qZrr5DwBx6bjh7g%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Essentials] (client) Error Telemetry logging: JEP in early draft review

Baptiste MATHUS
Quick heads-up everyone. So since last email, the proposal got accepted as JEP-304.

After a discussion yesterday with Tyler, we discussed a small adjustment to make JSON parsing and evolution more straightforward, making the `exception` field an object instead of a string as it was currently.

Feel free to provide feedback here or on the PR.

Thanks!

Le jeu. 5 avr. 2018 à 11:02, Baptiste Mathus <[hidden email]> a écrit :
Submitted as https://github.com/jenkinsci/jep/pull/77

Thanks everyone for the feedback.

2018-04-05 9:38 GMT+02:00 Baptiste Mathus <[hidden email]>:


2018-04-05 0:53 GMT+02:00 Jesse Glick <[hidden email]>:
Seems reasonable.

The implementation separates JSON records with a newline. This is not
apparent from the JEP; it would be nice to mention it, so that readers
do not wonder whether you are using some kind of streaming JSON parser
(which is unnecessary when each line is a well-formed JSON document).

Done.
 

`JsonFormatter.formatException` can be replaced with
`Functions.printThrowable`, which formats chained exceptions more
readably. The JEP proposal for a future `stacktrace` field neglects to
mention that this will not work as is for exceptions with causes
and/or suppressed exceptions. In general `Throwable.printStackTrace`
can be overridden in nontrivial ways, sometimes including information
not present in the `message` nor stack trace (though less so after
JENKINS-46140), so I would not recommend bothering with anything but a
plain printed string.

Ack. Adjusted the proposal too, thanks a lot.

I will submit the PR to official JEP later today I think. Still giving it a few hours for some more feedback but I don't expect much pushback in this area and for such a small JEP anyway.

Cheers

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANWgJS4rK2hFETfVrE%2B5u9bYy_SdRZO9zeH_0su43Qx1ChWCDg%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.