OTEL follow up part 2

[stanleypliu]

What type of PR is this? (check all applicable)

• 🍕 Feature
• 🐛 Bug Fix
• 📝 Documentation
• 🧑‍💻 Refactor
• ✅ Test
• 🤖 Build or CI
• ❓ Other (please specify)

Related Issue

N/A

Describe this PR

Follow up PR for #680 as that was merged before this commit could be pushed. INSTALL_MONITORING variable removed from compose.yaml as it shouldn't be responsible for controlling those deps. Instead, the user is now prompted to pass that along as a build argument.

Screenshots

N/A

Alternative Approaches Considered

N/A

Review Guide

N/A

Checklist before requesting a review

• 📖 Read the HOT Code of Conduct: https://docs.hotosm.org/code-of-conduct
• 👷‍♀️ Create small PRs. In most cases, this will be possible.
• ✅ Provide tests for your changes.
• 📝 Use descriptive commit messages.
• 📗 Update any related documentation and include any relevant screenshots.

[optional] What gif best describes this PR or how it makes you feel?

[spwoodcock]

Do you think we could possibly merge the two env vars into one?

For FieldTM we simply have the MONITORING var present in the prod github env vars - this results in both the dependencies being installed, and sentry being configured at server init

[stanleypliu]

Do you think we could possibly merge the two env vars into one?

For FieldTM we simply have the MONITORING var present in the prod github env vars - this results in both the dependencies being installed, and sentry being configured at server init

We can, although I originally wanted to retain the MONITORING variable just in case we want to have a different backend for sending the traces in future. Unless you think that this is premature optimisation 😄

[spwoodcock]

FieldTM works like this, based on the env:

MONITORING= dont install deps, dont enable monitoring on deploy

MONITORING=sentry install deps, configure sentry

MONITORING=openobserve install deps, configure generic otel openobserve

Would similar make sense here?

[stanleypliu]

FieldTM works like this, based on the env:

MONITORING= dont install deps, dont enable monitoring on deploy

MONITORING=sentry install deps, configure sentry

MONITORING=openobserve install deps, configure generic otel openobserve

Would similar make sense here?

Oh I see what you mean, for some reason I thought you were referring to merging SENTRY_DSN and MONITORING but now I realise you meant INSTALL_MONITORING and MONITORING. For the scope of this PR, I'll simply focus on removing INSTALL_MONITORING and then the task of adding a separate monitoring backend can be addressed in another PR.

[spwoodcock]

Thanks - sorry I think I have confused things more 😆

Installing deps in an entrypoint is an antipattern.

Does this help clarify what I mean? 🙏

https://github.com/hotosm/field-tm/blob/f8071e25c2209cf4bc1c6cf5350b9e6ef8eefb79/src/backend/Dockerfile#L95

[stanleypliu]

It does yeah. Didn't know it was an anti-pattern but looking at it again, it definitely makes more sense to install the deps in the Dockerfile.

Let me know if you agree with my most recent changes - we would also need to set MONITORING in the secrets for drone-tm.

[spwoodcock]

Its not a bad idea having EXTRA_GROUPS built dynamically. I assume the plan was to separate out sentry from other monitoring options 😃

Personally I can only foresee having two options here: sentry and generic OTEL, so maybe its overkill.

But it will function the same way in either case 👍