Closed joehillen closed 7 years ago
The only worry I have is that there's no warning or error when we send an Authorization
header across an unencrypted connection.
I'm open to suggestions there. However, most internet applications happily send Auth headers over non-TLS.
Should withElasticSearchLogger
call error
when there is a EsLogin
but the url doesn't start with https
? There would also need to be a way to override it for when you're on a trusted network or talking to localhost.
Sounds like a good idea to me. We'd have a knob in ElasticSearchConfig
with a name like esAllowAuthOverPlainHTTP
or something similar.
What's @23Skidoo / scrive's opinion on lens, it would be great to lensify config, so adding fields won't be backwards breaking change, if there's default?
@phadej Adding fields is not a backwards breaking change if your using defaultElasticSearchConfig
. I don't see how using lens would make that any different. You'd still have to use defaultElasticSearchConfig
if you were using lens.
@23Skidoo I've made the requested addition.
@joehillen it is breaking if constructor is exported. I personally dislike mentioning "don't use constructor, but defaultXXX
and setters", as it easy to not notice, and quite easy to enforce.
EDIT: and with lens the API is simpler if you omit constuctor, as you don't need to export getters and setters separately, like e.g. in http-client
But what does that have to do with lens?
We can make the constructor private and move it to a Internal
module. I'd prefer to avoid adding a dependency on lens
.
@joehillen edited the previous comment, @23Skidoo field lenses can be created without dependency. (Like it's done in Cabal
;))
lens seems like massive overkill for a flat 5 field config that is probably never modified.
Also, this is outside of scope of this PR.
It's related, as this PR forces major bump of the version. It's not biggie, but some solution would be great. And yes, the config is modified, how'd you set the server address otherwise?
Merged, thanks!
@phadej
field lenses can be created without dependency. (Like it's done in Cabal ;))
Can you remind me how the code for that looks like?
Three lines:
@phadej I'm okay with adding this stuff, but I'd prefer if it lived in its own module (e.g. Backend.ElasticSearch.Lens
).
I'll make a PR
We use https://www.elastic.co/cloud/as-a-service for logging, so we need HTTPS and Basic Auth in order to send logs there.