Skip to content
This repository has been archived by the owner on Aug 24, 2022. It is now read-only.

PMM-9632 Settings view usage. #374

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

Conversation

JiriCtvrtka
Copy link
Contributor

@JiriCtvrtka JiriCtvrtka commented May 4, 2022

PMM-9632

Build: SUBMODULES-0

  • Links to other linked pull requests (optional).

@@ -92,7 +81,7 @@ const (
commandTypeUpdate = "UPDATE"
commandTypeInsert = "INSERT"
commandTypeDelete = "DELETE"
commandTypeUtiity = "UTILITY"
commandTypeUtility = "UTILITY"
Copy link
Contributor Author

@JiriCtvrtka JiriCtvrtka May 10, 2022

Choose a reason for hiding this comment

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

Already fixed in previous PR, but its not merged yet.

Comment on lines 52 to 59
Name string `reform:"name"`
Value string `reform:"value"`
DefaultValue string `reform:"default_value"`
Description string `reform:"description"`
Minimum *int64 `reform:"minimum"`
Maximum *int64 `reform:"maximum"`
Options *string `reform:"options"`
Restart string `reform:"restart"`
Copy link
Member

Choose a reason for hiding this comment

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

let's keep only fields that we use in case PGSM team changes structure later we will have more PMM clients supported latest PGSM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -307,12 +242,12 @@ func (m *PGStatMonitorQAN) Run(ctx context.Context) {
m.changes <- agents.Change{Status: inventorypb.AgentStatus_STARTING}
}

lengthS := uint32(m.waitTime.Seconds())
lengthS := uint32(waitTime.Seconds())
buckets, err := m.getNewBuckets(ctx, lengthS)
Copy link
Member

Choose a reason for hiding this comment

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

I think you can call settings here and pass them to the getNewBuckets method, in that case, you will be able to support immediate change for waitTime as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

current, prev, err := m.monitorCache.getStatMonitorExtended(ctx, m.q, m.pgsmNormalizedQuery)
settings, err := m.getSettings()
if err != nil {
m.l.Errorf(err.Error())
Copy link
Member

Choose a reason for hiding this comment

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

If we get an error here, shouldn't we stop the method and return an error?
The same below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Moved before line 245 in pgstatmonitor.go as you required above and added continue there.

@JiriCtvrtka JiriCtvrtka requested a review from BupycHuk May 15, 2022 06:54
return false, errors.New("failed to get pgsm_normalized_query property")
}

if s[key].Value == "yes" {
Copy link
Member

Choose a reason for hiding this comment

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

If I got it right this value might be "1" as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes string "1" for older versions. Thank you.

@JiriCtvrtka JiriCtvrtka requested a review from BupycHuk May 16, 2022 05:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants