Skip to content

Implement service level profiling for command control logic #22

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Implement service level profiling for command control logic #22

wants to merge 5 commits into from

Conversation

artursarlo
Copy link

@artursarlo artursarlo commented Sep 16, 2025 •
edited
Loading

This PR aims to:

  • Introduce service level profiling capabilities to the command control logic;

This PR introduces changes to:

  • Database: new table schema for ProfilingHierarchyCommands;
  • POST /api/metrics/profile_request endpoint;
  • POST /api/metrics/heartbeat endpoint;

Description

Key features:

  • The user is able to create a profiling command for a whole service using the POST /metrics/profile_request endpoint;
  • The profile request is applied to all active hosts;
  • The profile request also creates a profiling command of high hierarchy for the whole service;
  • When a new host connects to the backend the heartbeat logic identifies if there is any active command for the host, if not it tries to generate a command based on a high hierarchy command that matches the new host;

Related Issue

N/A

Motivation and Context

The motivation behind this feature is:

  • Agent re-deployments are quite common;
  • New hosts are constantly retired and provisioned;
  • New hosts end up without no profiling command;
  • The ability to create profiling requests at service level solves this dynamic behavior of retirement / provisioning;
  • Also, this creates an ease of use to configure profiling for all active hosts under the same service;

How Has This Been Tested?

Tested locally on DEV environment

Screenshots

N/A

Checklist:

  • I have updated the relevant documentation.
  • I have added tests for new logic.

@artursarlo artursarlo changed the title Add new table to store service level profiling commands and more Implement service level profiling for command control logic Sep 23, 2025
@artursarlo artursarlo marked this pull request as ready for review September 23, 2025 16:57
@prashantbytesyntax
Copy link

good summary @artursarlo

prashantbytesyntax
prashantbytesyntax reviewed Sep 23, 2025
View reviewed changes
scripts/setup/postgres/gprofiler_recreate.sql
CREATE TABLE ProfilingHierarchyCommands (
ID bigserial PRIMARY KEY,
command_id uuid NOT NULL,
service_name text NULL,
Copy link

Choose a reason for hiding this comment

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

need process_name

Copy link
Author

Choose a reason for hiding this comment

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

Hummm, up until now we were only dealing with PIDs at the backend, not process names.
I can see the use case behind this, though (we might need to change the agent code to add process name instead of PID only).

The only reason I did not add PIDs into this table is because there is no reason to create a broad command based on PIDs (each host will have different PIDs)

src/gprofiler-dev/gprofiler_dev/postgres/db_manager.py
one_value=True,
return_dict=True,
)
existing_command = self.get_current_profiling_command(hostname, service_name)
Copy link

Choose a reason for hiding this comment

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

nice !

src/gprofiler-dev/gprofiler_dev/postgres/db_manager.py Outdated
) VALUES (
%(command_id)s::uuid, %(hostname)s, %(service_name)s, %(command_type)s,
%(final_request_ids)s::uuid[], %(final_config)s::jsonb,
%(new_command_id)s::uuid, %(hostname)s, %(service_name)s, %(command_type)s,
Copy link

Choose a reason for hiding this comment

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

probably modularize this into different method

src/gprofiler-dev/gprofiler_dev/postgres/db_manager.py
"""Create a stop command for an entire host"""
"""Create a stop command for a specific host or all active hosts of a service"""
if hostname is None: # Broadcast to all active hosts of the service
active_hosts = self.get_active_hosts(service_name)
Copy link

Choose a reason for hiding this comment

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

great to see we have already indexed idx_hostheartbeats_service_name, idx_profilinghierarchycommands_service_name

src/gprofiler/backend/routers/metrics_routes.py Outdated
hostname=heartbeat.hostname,
service_name=heartbeat.service_name,
)
if not current_command:
Copy link

Choose a reason for hiding this comment

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

can this abstracted away ? why does heartbeat needs to know about these methods

get_current_profiling_command
generate_profiling_command_from_hierarchy

So order will be get_current_profiling_command function -> inside this function, it will attempt to ask for host level commands and hieararchy level commands

prashantbytesyntax
prashantbytesyntax approved these changes Sep 23, 2025
View reviewed changes
Copy link

@prashantbytesyntax prashantbytesyntax left a comment

Choose a reason for hiding this comment

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

please also add read me

src/gprofiler-dev/gprofiler_dev/postgres/db_manager.py
logger.error(f"Failed to build hierarchy combined config for requests {request_ids}: {e}")
return {}

def _merge_hierarchy_configs(self, existing_config: Dict, new_request: Dict) -> Dict:
Copy link

Choose a reason for hiding this comment

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

not a blocker but merging logic could get complicated, i think we simply overwrite with the new config

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@prashantbytesyntax prashantbytesyntax prashantbytesyntax approved these changes

@mouralucas mouralucas Awaiting requested review from mouralucas

@ashokbytebytego ashokbytebytego Awaiting requested review from ashokbytebytego

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@artursarlo @prashantbytesyntax