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

SMQ-2648 - Add API and Repository implementation for the Client stats #2647

Merged
merged 5 commits into from
Jan 17, 2025

Conversation

felixgateru
Copy link
Contributor

@felixgateru felixgateru commented Jan 16, 2025

What type of PR is this?

This is a feature because it adds clients telemetry to the journals service

What does this do?

This pr adds a means to monitor information about clients

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

Have you included tests for your changes?

No,

Did you document any new/modified feature?

No

Notes

Copy link
Collaborator

@dborovcanin dborovcanin left a comment

Choose a reason for hiding this comment

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

It's OK to use Journal service as telemetry aggregator, but we should consider using some actual telemetry collectors for event processing. I.e. we can use journaling service as a final destination and apply access control and some processing to provide nice API, but what we need is an integration with telemetry collectors that will consume events, process them, store them, and provide search and filtering on them. Then, we need a background task to sync Journal service with them. Take a look at Elastic, Prometheus, InfluxDB.

journal/api/requests.go Outdated Show resolved Hide resolved
journal/middleware/authorization.go Outdated Show resolved Hide resolved
journal/middleware/metrics.go Outdated Show resolved Hide resolved
@dborovcanin dborovcanin changed the title SMQ- 2546 - Add clients telemetry to journals service SMQ-2648 - Add API and Repository implementation for the Client stats Jan 17, 2025
Copy link

codecov bot commented Jan 17, 2025

Codecov Report

Attention: Patch coverage is 23.07692% with 200 lines in your changes missing coverage. Please review.

Project coverage is 52.43%. Comparing base (5d75599) to head (8bb2a53).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
journal/postgres/telemetry.go 0.00% 89 Missing ⚠️
journal/mocks/repository.go 0.00% 45 Missing ⚠️
journal/middleware/authorization.go 0.00% 15 Missing ⚠️
journal/middleware/logging.go 0.00% 14 Missing ⚠️
journal/mocks/service.go 47.61% 5 Missing and 6 partials ⚠️
journal/middleware/tracing.go 0.00% 8 Missing ⚠️
journal/api/endpoint.go 64.70% 4 Missing and 2 partials ⚠️
journal/middleware/metrics.go 0.00% 6 Missing ⚠️
journal/service.go 0.00% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2647      +/-   ##
==========================================
+ Coverage   45.34%   52.43%   +7.08%     
==========================================
  Files         345       16     -329     
  Lines       44305      883   -43422     
==========================================
- Hits        20092      463   -19629     
+ Misses      22029      394   -21635     
+ Partials     2184       26    -2158     

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

@felixgateru felixgateru marked this pull request as ready for review January 17, 2025 11:31
@felixgateru felixgateru requested a review from a team as a code owner January 17, 2025 11:31
@@ -137,6 +137,17 @@ func (page JournalsPage) MarshalJSON() ([]byte, error) {
return json.Marshal(a)
}

type ClientsTelemetry struct {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use singular ClientTelemetry instead, since this is a telemetry for a single client.

type ClientsTelemetry struct {
ClientID string `json:"client_id"`
DomainID string `json:"domain_id"`
Connections []string `json:"connections"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is no need for connections, we already have it in the Client.

@@ -102,6 +104,71 @@ func (repo *repository) RetrieveAll(ctx context.Context, page journal.Page) (jou
return journalsPage, nil
}

func (repo *repository) SaveClientTelemetry(ctx context.Context, ct journal.ClientsTelemetry) error {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Create a separate file for telemetry implementation in telemetry,go.

…y, move repo impl to telemetry.go

Signed-off-by: Felix Gateru <[email protected]>
@dborovcanin dborovcanin merged commit fbbe5ff into absmach:main Jan 17, 2025
6 of 7 checks passed
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.

Add API and Repository implementation for the Client stats
2 participants