-
Notifications
You must be signed in to change notification settings - Fork 2k
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
[Event Hubs] Update logging to match design spec. #3531
Comments
@conniey If we are using ClientLogger everywhere, shouldn't that take care of logging as per spec? |
@srnagar This was around when we log use the client logger to log statements. I know there are times we use .info when it should be a .trace, and other times when we don't write errors. |
@conniey pls assign me this issue and elaborate a bit more on the the remaining things to do for this issues and also the affected modules |
@mclay Hey. I was taking a look at this task and it requires more in-depth knowledge of what exceptions are recoverable/normal and which are fatal. I am not sure how comfortable you'd feel delving into the inner workings of Event Hubs and our core AMQP library. This may be more than just a "up-for-grabs" task. One example is https://github.com/Azure/azure-sdk-for-java/blob/master/sdk/eventhubs/azure-messaging-eventhubs/src/main/java/com/azure/messaging/eventhubs/EventHubPartitionAsyncConsumer.java#L86 where, depending on the exception type, if it is an AmqpException, it could be retriable or not. So we would add some logic to log it appropriately. Let me know if you're still interested in helping out with this task. :) |
@conniey yes i am interested. Could you please give me some more information about the intention respectively goal of this task. |
@conniey do you have additional informations for me to consider or can i start right away with this one |
This task intends that if users are using our library, and something occurs in it, informational, trace, warning, error, they can diagnose it by setting AZURE_LOG_LEVEL environment variable. Most of the logging is done in azure-core-amqp. It was a carryover from our first SDK. so some of the logging may not match the specs. Yes. please go ahead and start. :) |
Thanks for the additional infos. Just to be sure - the due date is end of January right? |
I slotted it to be done end of January, but if it falls through, it won't be a problem because this is not a high priority task. There is a bunch of logging in place already; it's just not consistent and pretty arbitrary. :) So, I hope you don't feel rushed or anything. |
@mclay thanks for picking up this issue. 😄 Have you had time to make progress on this? |
@joshfree busy times.. if it's not too late for you i would like to start this one next |
Any progress on this? "Regular" setup logs way too verbose in my opinion. Just to give a few examples
Related issues: Looks like this seems to bother others as well in different ways. |
Update logging in azure-core-amqp and azure-messaging-eventhubs to align with guidelines in:
https://azure.github.io/azure-sdk/java_implementation.html#logging
The text was updated successfully, but these errors were encountered: