Add metadata header on requests #5190
Conversation
|
@Mpdreamz I've refactored this a fair bit taking your suggestions onboard. I'm happier with this approach now. Would welcome a check to see if it matches what you were thinking? |
|
Aye much cleaner! I am still unsure if we need: And For users wanting to inject we already have |
| SyncMetaDataHeader = new MetaDataHeader(clientVersionInfo, "es", false); | ||
| } | ||
|
|
||
| public MetaDataHeader AsyncMetaDataHeader { get; private set; } |
There was a problem hiding this comment.
| public MetaDataHeader AsyncMetaDataHeader { get; private set; } | |
| public MetaDataHeader AsyncMetaDataHeader { get; } |
There was a problem hiding this comment.
Yeah it could be. I want to read up on using records for older targets since I believe we may need to include code for a couple of attributes which may be required for them to work after compilation.
|
|
||
| namespace Elasticsearch.Net | ||
| { | ||
| internal sealed class MetaDataHeaders : IMetaDataHeaders |
There was a problem hiding this comment.
both internal and sealed are used very sparingly. Siding on the side of being public by default has some benefits.
Not sure we need IMetaDataHeaders if this class is public? It also allows us to unit test this in isolation.
There was a problem hiding this comment.
In this case, I think I probably agree 😄. I tend to lean the other way, internal and sealed until they need to be since we can change them freely without breaking changes to consumers. But when I've done that in other libs, it can cause some headaches for testing. Here, I think loosing the interface is better.
There was a problem hiding this comment.
@Mpdreamz In the latest update to this PR I've been able to avoid exposing this on the configuration entirely and been able to move its logic into the provider directly.
Add tests to ensure that observables make requests with the expected request meta data helper value, including the reindex helper with calls other helpers. These tests also highlight two errors in the snapshot and restore observable which have been fixed.
|
@Mpdreamz I think we're ready for a final review on this please. I've refactored and simplified this further on the basis of YAGNI. I also added |
Implement meta header for client requests
Adds a
x-elastic-client-metaheader to HTTP requests sent by Elasticsearch.NET. This header contains information about the runtime environment that is meant to allow analysing usage by collecting this information on the receiving side of requests, like a proxy server in front of ES.The information in this header has been specified by the clients team to be consistent across language clients. It contains the versions of the client, the language runtime (.NET in this case) and its HTTP library, and the use of the high level client.
This header is sent by default and can be disabled by setting
DisableMetaHeaderon theConnectionSettingspassed to the client.There is a little more work to do on this PR after an initial review of the approaches taken.
Remaining tasks: