Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
87 changes: 87 additions & 0 deletions backend/api/spec.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -1177,6 +1177,53 @@ paths:
description: Activity not found response
"500":
description: List activity error response
/instance-metrics/prometheus:
get:
description: Get latest instance stats
operationId: getLatestInstanceStats
security:
- oidcBearerAuth: []
- oidcCookieAuth: []
- githubCookieAuth: []
responses:
"200":
description: Get latest instance stats success response
content:
text/plain:
schema:
type: string
"500":
description: Get latest instance stats error response
/instance-metrics/json:
Copy link
Copy Markdown
Collaborator

@ervcz ervcz Apr 29, 2025

Choose a reason for hiding this comment

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

I'm wondering if we need the json sub-path, especially if we also define the response content type as json

Suggested change
/instance-metrics/json:
/instance-metrics:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In this case, I would propose to keep /instance-metrics/json and use /instance-metrics for the Prometheus endpoint to get closer from the default /metrics endpoint.

get:
description: Get instance stats
operationId: getInstanceStats
parameters:
- in: query
name: page
required: false
schema:
type: integer
minimum: 0
- in: query
name: perpage
required: false
schema:
type: integer
minimum: 10
security:
- oidcBearerAuth: []
- oidcCookieAuth: []
- githubCookieAuth: []
responses:
"200":
description: List instance stats response
content:
application/x-ndjson:
schema:
$ref: "#/components/schemas/instanceStats"
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.

should not this rather be referencing instanceStatsPage?

Suggested change
$ref: "#/components/schemas/instanceStats"
$ref: "#/components/schemas/instanceStatsPage"

"500":
description: Get instance stats error response
components:
schemas:
## request Body
Expand Down Expand Up @@ -1339,6 +1386,46 @@ components:
package_id:
type: string

instanceStatsPage:
type: object
required:
- totalCount
- count
- applications
properties:
totalCount:
type: integer
count:
type: integer
applications:
type: array
items:
$ref: "#/components/schemas/instanceStats"

instanceStats:
type: object
required:
- type
- channel
- version
- arch
- timestamp
- instances
properties:
type:
type: string
enum: ["instance_count"]
channel:
type: string
version:
type: string
arch:
type: string
timestamp:
type: string
count:
type: integer


## response
config:
Expand Down
19 changes: 16 additions & 3 deletions backend/pkg/api/instances.go
Original file line number Diff line number Diff line change
Expand Up @@ -723,11 +723,24 @@ func (api *API) instanceStatsQuery(t *time.Time, duration *time.Duration) *goqu.
return query
}

// GetInstanceStatsCount returns the total number of InstanceStats.
func (api *API) GetInstanceStatsCount() (int, error) {
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.

I noticed that the GetInstanceStats function accepts the perPage param as a uint64, but that is supposed to be a smaller number than the total count, and yet it reserves more memory, so there is a minor inconsistency in the usage of the types here, or am I wrong? Is go's default int type 32 bit?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Good catch. Let's stick to int.

query := goqu.From("instance_stats").Select(goqu.L("count(*)"))
return api.GetCountQuery(query)
}

// GetInstanceStats returns an InstanceStats table with all instances that have
// been previously been checked in.
func (api *API) GetInstanceStats() ([]InstanceStats, error) {
query, _, err := goqu.From("instance_stats").
Order(goqu.C("timestamp").Asc()).ToSQL()
func (api *API) GetInstanceStats(page, perPage uint64) ([]InstanceStats, error) {
page, perPage = validatePaginationParams(page, perPage)
limit, offset := sqlPaginate(page, perPage)

query, _, err := goqu.
From("instance_stats").
Limit(limit).
Offset(offset).
Order(goqu.C("timestamp").Asc()).
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.

I'm not sure if it matters or not, but is the ordering correct here? shouldn't we get the latest stats first?

ToSQL()
if err != nil {
return nil, err
}
Expand Down
6 changes: 3 additions & 3 deletions backend/pkg/api/instances_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -300,7 +300,7 @@ func TestUpdateInstanceStats(t *testing.T) {
a := newForTest(t)
defer a.Close()

instances, err := a.GetInstanceStats()
instances, err := a.GetInstanceStats(1, 100)
assert.NoError(t, err)
assert.Equal(t, 0, len(instances))

Expand Down Expand Up @@ -328,7 +328,7 @@ func TestUpdateInstanceStats(t *testing.T) {
err = a.UpdateInstanceStats(&ts, &elapsed)
assert.NoError(t, err)

instances, err = a.GetInstanceStats()
instances, err = a.GetInstanceStats(1, 100)
assert.NoError(t, err)
assert.Equal(t, 2, len(instances))

Expand Down Expand Up @@ -357,7 +357,7 @@ func TestUpdateInstanceStats(t *testing.T) {
err = a.UpdateInstanceStats(&ts3, &elapsed)
assert.NoError(t, err)

instances, err = a.GetInstanceStats()
instances, err = a.GetInstanceStats(1, 100)
assert.NoError(t, err)
assert.Equal(t, 5, len(instances))

Expand Down
38 changes: 38 additions & 0 deletions backend/pkg/api/metrics.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,12 @@ WHERE a.id = e.application_id AND e.event_type_id = et.id AND et.result = 0 AND
GROUP BY app_name
ORDER BY app_name
`, ignoreFakeInstanceCondition("e.instance_id"))

latestInstanceStatsSQL = `
SELECT channel_name, version, arch, timestamp, instances AS instances_count
FROM instance_stats
WHERE timestamp = (SELECT MAX(timestamp) FROM instance_stats)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Instead of doing raw SQL here, could we add a new function GetInstanceStatsLatest in pkg/api/instances.go and use that? This would also allow to add a test for this new function.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I had this logic initially in pkg/api/instances.go but decided to follow the convention above, though I agree with this suggestion

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.

do we have a index on timestamp?

`
)

type AppInstancesPerChannelMetric struct {
Expand Down Expand Up @@ -77,6 +83,38 @@ func (api *API) GetFailedUpdatesMetrics() ([]FailedUpdatesMetric, error) {
return metrics, nil
}

type LatestInstanceStatsMetric struct {
ChannelName string `db:"channel_name" json:"channel_name"`
Version string `db:"version" json:"version"`
Arch string `db:"arch" json:"arch"`
Timestamp string `db:"timestamp" json:"timestamp"`
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.

I guess it doesn't matter as we are just forwarding the value as text, but a go time type could also be used here if the driver supports the automatic conversion. Could be nice for later to change the presentation format of the timestamp.

InstancesCount int `db:"instances_count" json:"instances_count"`
}

func (api *API) GetLatestInstanceStatsMetrics() ([]LatestInstanceStatsMetric, error) {
var metrics []LatestInstanceStatsMetric
rows, err := api.db.Queryx(latestInstanceStatsSQL)
if err != nil {
return nil, fmt.Errorf("querying latest instance stats from SQL: %w", err)
}
defer rows.Close()

for rows.Next() {
var metric LatestInstanceStatsMetric
if err := rows.StructScan(&metric); err != nil {
return nil, fmt.Errorf("scanning instance stat metric: %w", err)
}

metrics = append(metrics, metric)
}

if err := rows.Err(); err != nil {
return nil, err
}

return metrics, nil
}

func (api *API) DbStats() sql.DBStats {
return api.db.Stats()
}
Loading
Loading