Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(otel): add otel to ingest API #342

Merged
merged 4 commits into from
Apr 23, 2024
Merged

feat(otel): add otel to ingest API #342

merged 4 commits into from
Apr 23, 2024

Conversation

ividito
Copy link
Collaborator

@ividito ividito commented Mar 22, 2024

Issue

#289

What?

  • Adds basic monitoring functionality to ingest API

Testing?

  • Deployed to temporary otel stack and viewed generated metrics and logs using POC Grafana instance

@smohiudd smohiudd requested a review from anayeaye March 28, 2024 20:29
Copy link
Collaborator

@anayeaye anayeaye left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approve but I am curious about the unhandled/unlogged exception err.

"""Handle exceptions that aren't caught elsewhere"""
metrics.add_metric(name="UnhandledExceptions", unit=MetricUnit.Count, value=1)
logger.exception("Unhandled exception")
return JSONResponse(status_code=500, content={"detail": "Internal Server Error"})
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we log the err here? f"Unhandled exception {err=}"?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that we have been doing the same in raster api and stac api so this may be for a discussion.

Copy link
Contributor

@smohiudd smohiudd Apr 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it makes sense to log the error but we could handle that in a separate PR including raster and stac. Or include it in this PR for all the services

@anayeaye
Copy link
Collaborator

@smohiudd @ividito I wonder if we should push this through before the auth PR #356 (we also may have to update the auth branch to resolve any diffs).

@ividito ividito merged commit 9f0289b into develop Apr 23, 2024
4 checks passed
botanical added a commit that referenced this pull request May 6, 2024
### Breaking
- #356

#### Breaking changes notes
Breaking: `VEDA_COGNITO_DOMAIN` configuration now required along with
one time administrator step to update existing user pool client allowed
callback urls with the ingest-api's URL

### Added
- #342
- #330
- #323

### Changed/Updated
- #355
- #340

### Fixed

1. - #367
2. - #365
3. - #361
4. - #360
5. - #358
8. - #345
9. - #344
12. - #339
13. - #338
14. - #337
15. - #335
16. - #334
17. - #331
20. - #329
21. - #327
22. - #326
23. - #325
24. - #324
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants