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: Loki Log Streaming #1385

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open

Conversation

amanji
Copy link
Collaborator

@amanji amanji commented Oct 9, 2024

No description provided.

@amanji amanji requested review from loneil and esune October 9, 2024 15:21
@amanji amanji linked an issue Oct 9, 2024 that may be closed by this pull request
@loneil loneil force-pushed the bcgov/feature/loki-log-streaming branch from 227c6a2 to 592789b Compare October 10, 2024 23:59
@loneil
Copy link
Collaborator

loneil commented Oct 10, 2024

Rebased and resolved package conflicts

@amanji amanji marked this pull request as ready for review October 11, 2024 21:33
@loneil
Copy link
Collaborator

loneil commented Oct 15, 2024

@amanji there's a pull request here that refactors some startup to a manage script. Any thoughts on if we merge that if that would cause any issues (or should have change to include) the docker compose startup instructions in this PR?

https://github.com/bcgov/traction/pull/1388/files

@amanji
Copy link
Collaborator Author

amanji commented Oct 15, 2024

Let's give it a try. I doubt there will be a huge impact

@amanji
Copy link
Collaborator Author

amanji commented Oct 15, 2024

@loneil we'd need to add this to the manage script:

docker compose -f docker-compose.logs.yml -f docker-compose.yml up

@amanji amanji force-pushed the bcgov/feature/loki-log-streaming branch from bea4259 to c8e0a77 Compare October 16, 2024 14:40
@loneil
Copy link
Collaborator

loneil commented Oct 16, 2024

FYI trying on the deployed PR and hitting start on the logs doesn't seem to show output or an error

See an aggregateError in the tenant UI backend logs:
image

@amanji
Copy link
Collaborator Author

amanji commented Oct 16, 2024

FYI trying on the deployed PR and hitting start on the logs doesn't seem to show output or an error

See an aggregateError in the tenant UI backend logs: image

We need to build in three more services for the logging. I'm assuming the deployed services dont account for those yet

@esune
Copy link
Member

esune commented Oct 18, 2024

FYI trying on the deployed PR and hitting start on the logs doesn't seem to show output or an error
See an aggregateError in the tenant UI backend logs: image

We need to build in three more services for the logging. I'm assuming the deployed services dont account for those yet

@i5okie do you have any suggestiono n how to set things up for the supporting services? i.e.: we list them as pre-requisites or do we include (optional) depenedncies in the Helm chart?

@i5okie
Copy link
Contributor

i5okie commented Oct 18, 2024

FYI trying on the deployed PR and hitting start on the logs doesn't seem to show output or an error
See an aggregateError in the tenant UI backend logs: image

We need to build in three more services for the logging. I'm assuming the deployed services dont account for those yet

@i5okie do you have any suggestiono n how to set things up for the supporting services? i.e.: we list them as pre-requisites or do we include (optional) depenedncies in the Helm chart?

In my opinion, I think Traction should work without throwing exceptions or failing when it runs without Loki backend being configured. As such, I think Loki should be an optional component setup separately by the user. I'd imagine, if loki endpoint is not provided/log-streaming is disabled, the feature should be turned off in someway. Or at least say logs unavailable in a friendly (not app crashing) manner on the page.

As for the promtail, it should be included as an optional sidecar.
We could put together documentation, with example configs on how to set Traction up with Loki.
I can help with the latter two.

Comment on lines +137 to +139
- SERVER_LOKI_URL=${SERVER_LOKI_URL}
- FRONTEND_TENANT_PROXY_URL=${FRONTEND_TENANT_PROXY_URL}
- FRONTEND_LOG_STREAM_URL=${FRONTEND_LOG_STREAM_URL}
Copy link
Member

Choose a reason for hiding this comment

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

These envvars need to be added to the chart as well

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See chart if its correct.

Signed-off-by: Akiff Manji <[email protected]>
Signed-off-by: Akiff Manji <[email protected]>
@amanji amanji requested a review from esune October 22, 2024 14:25
@amanji
Copy link
Collaborator Author

amanji commented Oct 22, 2024

FYI trying on the deployed PR and hitting start on the logs doesn't seem to show output or an error
See an aggregateError in the tenant UI backend logs: image

We need to build in three more services for the logging. I'm assuming the deployed services dont account for those yet

@i5okie do you have any suggestiono n how to set things up for the supporting services? i.e.: we list them as pre-requisites or do we include (optional) depenedncies in the Helm chart?

In my opinion, I think Traction should work without throwing exceptions or failing when it runs without Loki backend being configured. As such, I think Loki should be an optional component setup separately by the user. I'd imagine, if loki endpoint is not provided/log-streaming is disabled, the feature should be turned off in someway. Or at least say logs unavailable in a friendly (not app crashing) manner on the page.

As for the promtail, it should be included as an optional sidecar. We could put together documentation, with example configs on how to set Traction up with Loki. I can help with the latter two.

I'll update the PR to hide the log feature when the FRONTEND_LOG_STREAM_URL is not set.

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.

Log consumption for tenants
4 participants