[PLATFORM-1078]: Add explicit support for country metadata

[cpiemontese]

https://prima-assicurazioni-spa.myjetbrains.com/youtrack/issue/PLATFORM-1078

Add support for country

[cpiemontese]

I feel like making the country an Option has led to a shitty implementation inside, but let me know

I'll start tackling the env thing

[MaeIsBad]

I feel like making the country an Option has led to a shitty implementation inside, but let me know

actually maybe we can just require country to be set in the constructor? It will be a breaking change we actually care about but I think that every service having a name and a country isn't too big of a requirement

[cpiemontese]

Since it's technically breaking anyway, I tried refactoring things a bit

I'm not completely convinced of the panic! in the load_... methods and if it makes sense to implement loading from env this way, but it's the best I could come up with

[oliverbrowneprima]

These load_ functions seem a little like they're doing too much - most apps will have their own way to load environment variables into a config struct, or from a file, or something, and ideally we don't need to provide it at all

If we are providing it, panicking is surprising to me - this is library code, so maybe returning a Result<Self, Self>, where in the Err side is the unmodified builder and the Ok side is the modified, would be best?

[cpiemontese]

Agreed, we talked about reading this stuff from env but now that I've implemented it I'm not so sure

I thought about using Result but I don't like that it kinda breaks the builder pattern, I like the idea of the type state more honestly

[oliverbrowneprima]

I mean even with a type-state approach, fallible operations are still fallible, and my view is that fallible operations in library code should return a result (with type-state approaches it would just be a Result<Builder<WithCountry>, Builder<WithoutCountry>>, for example)

I'm not totally against it, but if it does panic you /really/ need to mention that it does, and why, in the doc comment

[cpiemontese]

It turns out that using types instead of phantom types is good! (see: https://www.greyblake.com/blog/builder-with-typestate-in-rust/)

This has escalated quickly, but we now have required the environment and the country, which unfortunately is a breaking change.

I removed the load functions because I don't really like a builder which reads from env, the users can do it and try to parse the country using Country::from_str

[cpiemontese]

I tried the typed builder create but it doesn't fully support "pre-setting" certain parameters (i.e. the formatter). Once you set something you can't set it anymore

I tried setting the formatter as an option but it makes the interface worse down the line

As it stands the PR introduces the following changes

• country is supported and explicitly required
• env is explicitly required

I chose not to introduce the loading from env because it doesn't seem like a responsibility of prima_tracing.rs

[cottinisimone]

Do we still have Qa?

[MaeIsBad]

We plan to get rid of it but for now it's still a thing

[MaeIsBad]

No notes from me, I would wait to see if anyone else has anything to say

[oliverbrowneprima]

LGTM