EventHub + ServiceHub with EventProducerIdentifier #174
Conversation
|
@ssteiner Care to review my changes for adding the new Proxy-settings to ServiceBusTarget + EventHubTarget I have updated |
snakefoot
force-pushed
the
master
branch
3 times, most recently
from
07957a4 to
a2b2617
Compare
|
@ssteiner Polite poke for review of the suggested changes of |
|
thanks @snakefoot |
|
Nope. I don't have any more to add for now.
|
|
@snakefoot : did you not see the review I started? I spent an hour last Monday reviewing and commenting. Still seeing it all on this page |
|
No I did not see anything. I'm guessing a GitHub Review not yet published / completed is hidden from everyone else.
|
Weird - I guess I'm not that used to the Github interface as we use Azure Devops internally. Here's screnshots of what I did 
It says pending, but I don't see how what more I could do. I can edit or delete the individual comments, I see no 'publish' button or similar. |
|
Believe the button used for starting the review must be clicked again for completing the review.
I seldom use the GitHub review feature but just writes comments directly to the pull request
|
| _connectionString_ - Azure Blob Storage connection string from your storage account. Required unless using `ServiceUri`. | ||
|
|
||
| _serviceUri_ - Uri to reference the blob service (e.g. https://{account_name}.blob.core.windows.net). Input for `BlobServiceClient`. Required, when `connectionString` is not configured. Overrides `connectionString` when both are set. | ||
| _serviceUri_ - Uri to reference the blob service (e.g. https://{account_name}.blob.core.windows.net). Alternative to ConnectionString, where Managed Identiy is applied from DefaultAzureCredential. |
There was a problem hiding this comment.
That's not entirely correct, is it? It not only applies to a Managed Identity (note the typo there.. you missed one t), ServiceUrl is also required when using sharedAccessSignature, _accessKey_, clientAuthId for authentication.
There was a problem hiding this comment.
By default it will use DefaultAzureCredential unless something else has been specified using the many authentication-options.
| _connectionString_ - Azure storage connection string. [Layout](https://github.com/NLog/NLog/wiki/Layouts) Required. | ||
| _connectionString_ - Azure storage connection string. [Layout](https://github.com/NLog/NLog/wiki/Layouts) Required unless using `ServiceUri`. | ||
|
|
||
| _serviceUri_ - Alternative to ConnectionString, where Managed Identiy is applied from DefaultAzureCredential. |
There was a problem hiding this comment.
see comment on Blob Storage target
There was a problem hiding this comment.
By default it will use DefaultAzureCredential unless something else has been specified using the many authentication-options.
| if (NoProxy) | ||
| return null; | ||
| if (string.IsNullOrEmpty(Address)) | ||
| return defaultProxy; |
There was a problem hiding this comment.
this prevents the two websockets targets from using custom credentials with the default proxy, no?
The other targets go through CreateHttpClientTransport where we check Address for null (and then go to the else case in the if-then-else), but the two websocket targets directly access CreateWebProxy and thus act differently.
For the websockets case, since we have no HttpClientHandler to work with, we could use System.Net.WebRequest.GetSystemWebProxy(); to get the system proxy, and then can set the default or custom Credentials on it.
There was a problem hiding this comment.
I was not able to see how one could override the HttpClient-Proxy-Credentials for ServiceBusTarget + EventHubTarget.
You are welcome to create pull-request to improve on this.
There was a problem hiding this comment.
For .NET Core, you can get the default proxy and set credentials on it by accessing HttpClient.DefaultProxy. And that allows you to set the credentials (and thus, default credentials). So at least for .NET core it's reasonably simple. I guess some things you can't just properly do on netstandard2. (there's always WebProxy.GetDefaultPRoxy for netstandard, but that's deprecated.. so I'd rather say netstandard2.0 is a bit limited and leave it at that).
There was a problem hiding this comment.
Then we are back to my perception that NLog is a guest in the application-house, and should not be modifying global application-state like HttpClient.DefaultProxy or WebProxy.GetDefaultProxy
Hmm... I think I figured it out now. Imho, the usability of Git isn't good on this - I had to look for a manual and then only got the right place by accident. While some of my review is questions only, at least the last one is about proxy functionality that is missing from the webSocket targets as it was merged now. |
|
Created #175 as followup to the code-review-comments. @ssteiner I appreciate your goal of making the documentation as good as the code. I have become used to the code slowly diverting from the documentation, so I usually make the documentation as terse as possible to avoid making false promises. But I'm surprised that you are using Microsoft as an example of good API-documentation, since I often find myself looking at the actual source-code since the Microsoft API-docs are often lacking. Regarding Github-review then I guess it makes sense, when reviewing code-changes over 100 source-files, and need to make more than 50 comments to reduce the noise-level for each comment. But really hope I never have to go through that scenario, since I like pull-requests small and nimble. I only use the Github-Review-logic to mark pull-request as approved without comments, and write individual comments until reaching that point. |
3 participants
Resolves #170 for NLog EventHubTarget and NLog ServiceBusTarget (other targets already fixed by #172)