Skip to content

Wip 422 telemetry#258

Merged
bstuder merged 9 commits into
developfrom
wip_422_telemetry
Apr 9, 2025
Merged

Wip 422 telemetry#258
bstuder merged 9 commits into
developfrom
wip_422_telemetry

Conversation

@bstuder
Copy link
Copy Markdown
Collaborator

@bstuder bstuder commented Apr 9, 2025

Add telemetry elements: loki/tempo/grafana/otlp-collector

@bstuder bstuder self-assigned this Apr 9, 2025
@PaulineMauryL
Copy link
Copy Markdown
Member

PaulineMauryL commented Apr 9, 2025

Coverage report

This PR does not seem to contain any modification to coverable code.

Comment thread devenv.nix Outdated
loki_http_port = 13100;
# loki_grpc_port = 19096;
otlp_host = "localhost";
otlp_grpc_port = 4317;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

There's some duplicates here (eg. otlp_host and otel_port at line 30).

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Maybe mark these as "Do not change" since the docker compose file uses fixed values. (Also did you use the same values as harcoded in there?)

Comment thread devenv.nix
# Environment variable available inside devenv
env = {
GREET = "Lomas env";
TELEMETRY = 1;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nice

Comment thread devenv.nix
scripts.pip-fix.exec = ''
pushd $DEVENV_ROOT
uv pip compile pyproject.toml --annotation-style line --all-extras -o requirements.txt
uv pip compile pyproject.toml --annotation-style line --all-extras $@ | ${pkgs.gnused}/bin/sed -re '/^-e file:/d' > requirements.txt
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

👍

Comment thread devenv.nix Outdated
}
];
};
# cheeky override of postgres statup command to force a aclean start
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

👍

Comment thread devenv.nix
${config.scripts.ut.exec}
'';

scripts.yelp.exec = ''
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nice!
Can we update the doc with this? I think it would be CONTRIBUTING.md and server/CONTRIBUTING.md. Otherwise we can also do it in another pr.

Copy link
Copy Markdown
Collaborator

@damienbfs damienbfs left a comment

Choose a reason for hiding this comment

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

Very nice!

@bstuder bstuder force-pushed the wip_422_telemetry branch from 6025ef5 to a30028a Compare April 9, 2025 09:16
@bstuder bstuder force-pushed the wip_422_telemetry branch from 89772a6 to 0872ddd Compare April 9, 2025 09:40
@bstuder bstuder merged commit 1c756c6 into develop Apr 9, 2025
@bstuder bstuder deleted the wip_422_telemetry branch April 9, 2025 09:54
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