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

MF - 1758 - Sync With Benchmark Testing #1912

Merged
merged 3 commits into from
Oct 24, 2023

Conversation

rodneyosodo
Copy link
Member

@rodneyosodo rodneyosodo commented Oct 4, 2023

What does this do?

  1. Add trace ratio for traces MF_JAEGER_TRACE_RATIO i.e. don't always trace every request. 1.0 means always sample while 0.0 means never sample. This has proven useful for benchmark testing as when the ratio is 1.0 the Jaeger container uses a lot of memory
  2. Add Postgres max connection MF_POSTGRES_MAX_CONNECTIONS. This is to increase the connected clients to Postgres i.e. previously a max of 100 clients was not ideal for testing

Which issue(s) does this PR fix/relate to?

Related to #1758

List any changes that modify/break current functionality

None

Have you included tests for your changes?

No

Did you document any new/modified functionality?

No

Notes

Linked with absmach/benchmark#10

@rodneyosodo rodneyosodo changed the title NOISSUE - Sync With Benchmark Testing MF - 1758 - Sync With Benchmark Testing Oct 4, 2023
@rodneyosodo rodneyosodo force-pushed the noissue-benchmark branch 4 times, most recently from 85f90e5 to bbd56f8 Compare October 18, 2023 08:38
@rodneyosodo rodneyosodo force-pushed the noissue-benchmark branch 6 times, most recently from f667c72 to 6909e73 Compare October 23, 2023 14:24
@rodneyosodo rodneyosodo marked this pull request as ready for review October 23, 2023 14:26
@rodneyosodo rodneyosodo requested a review from a team as a code owner October 23, 2023 14:26
docker/.env Outdated

## Call home
MF_SEND_TELEMETRY=true

## Postgres
MF_POSTGRES_MAX_CONNECTIONS=100000
Copy link
Contributor

Choose a reason for hiding this comment

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

Why we should have 100,000 connections by default ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Reduced to 100 i.e default

Copy link
Contributor

@arvindh123 arvindh123 left a comment

Choose a reason for hiding this comment

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

Expect POSTGRES_MAX_CONNECTIONS limit everything look good.

Some thing can we have like this below

In .env

--  MF_POSTGRES_MAX_CONNECTIONS=100

In docker-compose.yaml

--       MF_POSTGRES_MAX_CONNECTIONS: ${MF_POSTGRES_MAX_CONNECTIONS}
++     MF_POSTGRES_MAX_CONNECTIONS: ${MF_POSTGRES_MAX_CONNECTIONS:-100}}

Copy link
Contributor

@arvindh123 arvindh123 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@drasko drasko left a comment

Choose a reason for hiding this comment

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

LGTM

rodneyosodo and others added 3 commits October 24, 2023 17:23
This adds a new environment variable `MF_JAEGER_TRACE_RATIO` to the `docker/.env` file. The variable is used to set the ratio of requests traced.

Additionally, this commit also adds a new environment variable `MF_POSTGRES_MAX_CONNECTIONS` for configuring the maximum number of connections for the Postgres database.

These changes are made to enhance the configuration and scalability of the core services.

Signed-off-by: Rodney Osodo <[email protected]>
@drasko drasko force-pushed the noissue-benchmark branch from 297d99a to ce45014 Compare October 24, 2023 15:24
@drasko drasko merged commit 8b185d2 into absmach:master Oct 24, 2023
1 check passed
@rodneyosodo rodneyosodo deleted the noissue-benchmark branch October 22, 2024 08:14
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.

5 participants