PMM-10684 Clickhouse v2. #1542

JiriCtvrtka · 2023-01-04T13:16:34Z

codecov · 2023-01-04T13:20:12Z

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 44.47%. Comparing base (66f7907) to head (3a02a10).

Files	Patch %	Lines
managed/utils/envvars/parser.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##               v3    #1542      +/-   ##
==========================================
- Coverage   44.48%   44.47%   -0.01%     
==========================================
  Files         368      368              
  Lines       35586    35582       -4     
==========================================
- Hits        15829    15825       -4     
  Misses      18130    18130              
  Partials     1627     1627

Flag	Coverage Δ
admin	`11.65% <ø> (ø)`
agent	`52.74% <ø> (ø)`
managed	`46.49% <0.00%> (-0.01%)`	⬇️
vmproxy	`66.66% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ademidoff

👍

ademidoff · 2024-03-22T08:15:08Z

managed/services/supervisord/supervisord.go

@@ -425,8 +425,6 @@ func (s *Service) marshalConfig(tmpl *template.Template, settings *models.Settin
 	clickhouseDatabase := getValueFromENV("PERCONA_TEST_PMM_CLICKHOUSE_DATABASE", defaultClickhouseDatabase)
 	clickhouseAddr := getValueFromENV("PERCONA_TEST_PMM_CLICKHOUSE_ADDR", defaultClickhouseAddr)
 	clickhouseDataSourceAddr := getValueFromENV("PERCONA_TEST_PMM_CLICKHOUSE_DATASOURCE_ADDR", defaultClickhouseDataSourceAddr)
-	clickhousePoolSize := getValueFromENV("PERCONA_TEST_PMM_CLICKHOUSE_POOL_SIZE", "")


Did they remove these two params from the client?

OK, I see the commit message - it's no longer supported )

In v2 pool size and block size changed. It is also recommended to set them agains exact DB.

See: https://clickhouse.com/docs/en/integrations/go
MaxIdleConns should be alternative for pool size
BlockBufferSize for block size

But our block size default was 10 000 and BlockBufferSize did not accept such number, only lower ones, like 100.

time="2024-03-22T07:35:19.278+00:00" level=info msg="DSN: clickhouse://127.0.0.1:9000/pmm?block_buffer_size=10000&max_idle_conns=2" component=main stdlog: Connection: strconv.ParseUint: parsing "10000": value out of range

So seems it is slightly different. MaxIdleConns should behave same like pool size, but since both of them are with prefix "PERCONA_TEST" I decided to remove them.

Should the PR be against v3 branch?

My fault. Thanks for notice that.

Don't we treat variables prefixed with PERCONA_TEST_ as non-public, internal ones?

I'm referring to

pmm/managed/CONTRIBUTING.md

Line 54 in 70d5afd

## Available test environment variables:

I mean they should be used in non-prod environments, i.e. test environments.

You are right, seems like we can merge it to PMM 2 as well

@BupycHuk So should I change it to v2 (ticket and PR)? Thanks.

If yes, I will keep this one to merge it into v3 and for v2 I will create another PR and update ticket.

let's do it for v3 now, later we can backport to v2

BupycHuk

looks good to me, but I think it's better to do it in v3 branch since we are removing some env variables.

BupycHuk · 2024-03-22T08:56:31Z

managed/services/supervisord/supervisord.go

@@ -425,8 +425,6 @@ func (s *Service) marshalConfig(tmpl *template.Template, settings *models.Settin
 	clickhouseDatabase := getValueFromENV("PERCONA_TEST_PMM_CLICKHOUSE_DATABASE", defaultClickhouseDatabase)
 	clickhouseAddr := getValueFromENV("PERCONA_TEST_PMM_CLICKHOUSE_ADDR", defaultClickhouseAddr)
 	clickhouseDataSourceAddr := getValueFromENV("PERCONA_TEST_PMM_CLICKHOUSE_DATASOURCE_ADDR", defaultClickhouseDataSourceAddr)
-	clickhousePoolSize := getValueFromENV("PERCONA_TEST_PMM_CLICKHOUSE_POOL_SIZE", "")


Should the PR be against v3 branch?

BupycHuk · 2024-03-22T08:59:53Z

qan-api2/db_test.go

 		}

 		db := createDB(dsn)

-		require.Equal(t, db, nil, "Check connection after we create database")
+		require.Equal(t, nil, db, "Check connection after we create database")


require.Nil?

idoqo · 2024-06-14T07:56:08Z

docs/process/v2_to_v3_environment_variables.md

@@ -26,11 +26,9 @@ Below is a list of affected variables and their new names.
 | `PERCONA_TEST_CHECKS_PUBLIC_KEY`              |                                            | Removed in PMM v3, use `PMM_DEV_PERCONA_PLATFORM_PUBLIC_KEY` |
 | `PERCONA_TEST_NICER_API`                      | `PMM_NICER_API`                            |                                                              |
 | `PERCONA_TEST_PMM_CLICKHOUSE_ADDR`            | `PMM_CLICKHOUSE_ADDR`                      |                                                              |
-| `PERCONA_TEST_PMM_CLICKHOUSE_BLOCK_SIZE`      | `PMM_CLICKHOUSE_BLOCK_SIZE`                |                                                              |


I think that since the variables still exist in v2, it's better we keep them here and mention that they're now removed in v3 in the comments column.

Added back with description.

qan-api2/models/metrics.go

JiriCtvrtka force-pushed the PMM-10684-clickhouse-v2 branch from 7579b3b to c7da376 Compare March 21, 2024 14:17

JiriCtvrtka mentioned this pull request Mar 21, 2024

PMM-12551 Clickhouse v2. #2920

Closed

JiriCtvrtka changed the title ~~PMM-10684 Update clickhouse version to v2.~~ PMM-10684 Clickhouse v2. Mar 21, 2024

JiriCtvrtka mentioned this pull request Mar 21, 2024

PMM-10684 Clickhouse v2. Percona-Lab/pmm-submodules#3596

Closed

1 task

JiriCtvrtka marked this pull request as ready for review March 22, 2024 08:07

JiriCtvrtka requested review from a team as code owners March 22, 2024 08:07

JiriCtvrtka requested review from ademidoff, BupycHuk and idoqo and removed request for a team March 22, 2024 08:07

ademidoff approved these changes Mar 22, 2024

View reviewed changes

BupycHuk reviewed Mar 22, 2024

View reviewed changes

JiriCtvrtka changed the base branch from main to v3 March 22, 2024 09:34

PMM-12551 Clickhouse v2.

e1135ac

JiriCtvrtka changed the base branch from v3 to main March 22, 2024 09:41

JiriCtvrtka changed the base branch from main to v3 March 22, 2024 09:41

JiriCtvrtka added 4 commits March 22, 2024 10:42

PMM-12551 Fix for new connection string format.

77e9a3f

PMM-12551 Another fix for new connection string.

7cd9ca9

PMM-10684 Remove block size, pool size (not supported anymore).

92449b1

PMM-10684 Supervisor fix.

6e644ea

JiriCtvrtka force-pushed the PMM-10684-clickhouse-v2 branch from 00c0d01 to 6e644ea Compare March 22, 2024 09:45

PMM-10684 Small change.

05f03fa

JiriCtvrtka requested a review from BupycHuk March 22, 2024 09:48

BupycHuk approved these changes Mar 22, 2024

View reviewed changes

Merge branch 'v3' into PMM-10684-clickhouse-v2

0ee113b

JiriCtvrtka force-pushed the PMM-10684-clickhouse-v2 branch from 6c41623 to 0ee113b Compare June 11, 2024 10:56

PMM-10684 QAN fix after merge.

d7ab083

JiriCtvrtka added 3 commits June 11, 2024 13:08

PMM-10684 Remove outdated env variables.

705bae9

PMM-10684 Tidy.

bcfe01b

PMM-10684 Fix.

fa38077

idoqo approved these changes Jun 14, 2024

View reviewed changes

PMM-10684 Fix for plan.

cbda0a1

percona-robot reviewed Jun 17, 2024

View reviewed changes

qan-api2/models/metrics.go Outdated Show resolved Hide resolved

JiriCtvrtka and others added 3 commits June 17, 2024 11:33

PMM-10684 Lint.

4e6d768

PMM-10684 Env var description.

b74cdc9

Merge branch 'v3' into PMM-10684-clickhouse-v2

3a02a10

JiriCtvrtka enabled auto-merge (squash) June 18, 2024 09:09

JiriCtvrtka merged commit cd40e7a into v3 Jun 18, 2024
28 checks passed

JiriCtvrtka deleted the PMM-10684-clickhouse-v2 branch June 18, 2024 09:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PMM-10684 Clickhouse v2. #1542

PMM-10684 Clickhouse v2. #1542

JiriCtvrtka commented Jan 4, 2023 •

edited

Loading

codecov bot commented Jan 4, 2023 •

edited

Loading

ademidoff left a comment

ademidoff Mar 22, 2024

ademidoff Mar 22, 2024

JiriCtvrtka Mar 22, 2024

BupycHuk Mar 22, 2024

JiriCtvrtka Mar 22, 2024

ademidoff Mar 22, 2024

ademidoff Mar 22, 2024

BupycHuk Mar 22, 2024

JiriCtvrtka Mar 22, 2024 •

edited

Loading

BupycHuk Mar 22, 2024

BupycHuk left a comment

BupycHuk Mar 22, 2024

BupycHuk Mar 22, 2024

JiriCtvrtka Mar 22, 2024

idoqo Jun 14, 2024

JiriCtvrtka Jun 17, 2024

PMM-10684 Clickhouse v2. #1542

PMM-10684 Clickhouse v2. #1542

Conversation

JiriCtvrtka commented Jan 4, 2023 • edited Loading

codecov bot commented Jan 4, 2023 • edited Loading

Codecov Report

ademidoff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JiriCtvrtka Mar 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BupycHuk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JiriCtvrtka commented Jan 4, 2023 •

edited

Loading

codecov bot commented Jan 4, 2023 •

edited

Loading

JiriCtvrtka Mar 22, 2024 •

edited

Loading