Skip to content

feat(logs): Introduce logs command #2664

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

Merged
merged 20 commits into from
Aug 6, 2025
Merged
Show file tree
Hide file tree
Changes from 3 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
116 changes: 114 additions & 2 deletions src/api/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1226,6 +1226,29 @@ impl<'a> AuthenticatedApi<'a> {
Ok(rv)
}

/// Fetch organization events from the specified dataset
pub fn fetch_organization_events(
&self,
org: &str,
options: &FetchEventsOptions,
) -> ApiResult<Vec<LogEntry>> {
let params = options.to_query_params();
let url = format!(
"/organizations/{}/events/?{}",
PathArg(org),
params.join("&")
);

let resp = self.get(&url)?;

if resp.status() == 404 {
return Err(ApiErrorKind::OrganizationNotFound.into());
}

let logs_response: LogsResponse = resp.convert()?;
Ok(logs_response.data)
}

/// List all issues associated with an organization and a project
pub fn list_organization_project_issues(
&self,
Expand Down Expand Up @@ -1390,6 +1413,78 @@ impl<'a> AuthenticatedApi<'a> {
}
}

/// Available datasets for fetching organization events
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub enum Dataset {
/// Our logs dataset
OurLogs,
}

impl Dataset {
/// Returns the string representation of the dataset
fn as_str(&self) -> &'static str {
match self {
Dataset::OurLogs => "ourlogs",
}
}
}

impl fmt::Display for Dataset {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
write!(f, "{}", self.as_str())
}
}

/// Options for fetching organization events
pub struct FetchEventsOptions<'a> {
/// Dataset to fetch events from
pub dataset: Dataset,
/// Fields to include in the response
pub fields: &'a [&'a str],
/// Project ID to filter events by
pub project_id: Option<&'a str>,
/// Cursor for pagination
pub cursor: Option<&'a str>,
/// Query string to filter events
pub query: Option<&'a str>,
/// Number of events per page (default: 100)
pub per_page: Option<usize>,
/// Time period for stats (default: "1h")
pub stats_period: Option<&'a str>,
/// Sort order (default: "-timestamp")
pub sort: Option<&'a str>,
Copy link
Member

Choose a reason for hiding this comment

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

For now, all of these fields appear to always be being set to a Some variant, so the default value never ends up being used.

Therefore, I would like to see the struct fields be made non-optional, so we can also get rid of the defaults in the code, and make the API simpler. We can always later make the fields optional, as needed.

Suggested change
/// Query string to filter events
pub query: Option<&'a str>,
/// Number of events per page (default: 100)
pub per_page: Option<usize>,
/// Time period for stats (default: "1h")
pub stats_period: Option<&'a str>,
/// Sort order (default: "-timestamp")
pub sort: Option<&'a str>,
/// Query string to filter events
pub query: &'a str,
/// Number of events per page
pub per_page: usize,
/// Time period for stats
pub stats_period: &'a str,
/// Sort order
pub sort: &'a str,

Copy link
Member

@shellmayr shellmayr Aug 6, 2025

Choose a reason for hiding this comment

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

@szokeasaurusrex Calls to the endpoint don't need to have a query, stats_period, per_page or sort. What are we achieving by making them non-optional?

Copy link
Member

@szokeasaurusrex szokeasaurusrex Aug 6, 2025

Choose a reason for hiding this comment

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

I want to keep the Sentry CLI code simple. Making these optional means we have to add code paths to Sentry CLI, which for now, are not being used, because we always set the query, stats_period, per_page, and sort. Rust, however, still forces us to handle the cases of these being None (even though, in reality, they never are None).

As a result, we have code paths, which are completely unused, to handle the None cases. In the case of the query, we only provide it if it is there. For stats_period, per_page, and sort, we have default values defined, which are never used. From reading the code, however, it is not immediately clear that these default values are never used.

The problem with keeping them as Option is just that it increases complexity and it makes it harder to see where the actual values get defined. I understand that the values are optional from the server perspective, but that absolutely does not mean that we need to make them optional in the CLI's API

Copy link
Member

Choose a reason for hiding this comment

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

@shellmayr As an alternative, I would also be okay with just hardcoding the defaults and removing these fields from the struct completely. I'm okay with whatever you think is more reasonable, just want to avoid the Option when its adding currently unnecessary complexity by handling cases which never occur.

}

impl<'a> FetchEventsOptions<'a> {
/// Generate query parameters as a vector of strings
pub fn to_query_params(&self) -> Vec<String> {
let mut params = vec![format!("dataset={}", QueryArg(self.dataset.as_str()))];

for field in self.fields {
params.push(format!("field={}", QueryArg(field)));
}

if let Some(cursor) = self.cursor {
params.push(format!("cursor={}", QueryArg(cursor)));
}

if let Some(project_id) = self.project_id {
params.push(format!("project={}", QueryArg(project_id)));
}

if let Some(query) = self.query {
params.push(format!("query={}", QueryArg(query)));
}

params.push(format!("per_page={}", self.per_page.unwrap_or(100)));
params.push(format!("statsPeriod={}", self.stats_period.unwrap_or("1h")));

params.push(format!("sort={}", self.sort.unwrap_or("-timestamp")));

Copy link

Choose a reason for hiding this comment

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

Bug: Default Parameter Inclusion Bug

The FetchEventsOptions::to_query_params method incorrectly includes per_page, statsPeriod, and sort parameters in the query string with default values, even when their Option fields are None. This is due to the use of unwrap_or(), which forces their inclusion, contradicting their intended optional behavior and the API's design. In contrast, cursor and query parameters are correctly added only when explicitly provided.

Fix in Cursor Fix in Web

params
}
}

impl RegionSpecificApi<'_> {
fn request(&self, method: Method, url: &str) -> ApiResult<ApiRequest> {
self.api
Expand Down Expand Up @@ -2343,7 +2438,7 @@ pub struct ProcessedEvent {
pub tags: Option<Vec<ProcessedEventTag>>,
}

#[derive(Clone, Debug, Deserialize)]
#[derive(Clone, Debug, Deserialize, Serialize)]
pub struct ProcessedEventUser {
#[serde(skip_serializing_if = "Option::is_none")]
pub id: Option<String>,
Expand Down Expand Up @@ -2377,7 +2472,7 @@ impl fmt::Display for ProcessedEventUser {
}
}

#[derive(Clone, Debug, Deserialize)]
#[derive(Clone, Debug, Deserialize, Serialize)]
pub struct ProcessedEventTag {
pub key: String,
pub value: String,
Expand All @@ -2401,3 +2496,20 @@ pub struct Region {
pub struct RegionResponse {
pub regions: Vec<Region>,
}

/// Response structure for logs API
#[derive(Debug, Deserialize)]
struct LogsResponse {
data: Vec<LogEntry>,
}

/// Log entry structure from the logs API
#[derive(Debug, Deserialize)]
pub struct LogEntry {
#[serde(rename = "sentry.item_id")]
pub item_id: String,
pub trace: Option<String>,
pub severity: Option<String>,
pub timestamp: String,
pub message: Option<String>,
}
2 changes: 2 additions & 0 deletions src/commands/derive_parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ use crate::utils::auth_token::AuthToken;
use crate::utils::value_parsers::{auth_token_parser, kv_parser};
use clap::{command, ArgAction::SetTrue, Parser, Subcommand};

use super::logs::LogsArgs;
use super::send_metric::SendMetricArgs;

#[derive(Parser)]
Expand Down Expand Up @@ -32,5 +33,6 @@ pub(super) struct SentryCLI {

#[derive(Subcommand)]
pub(super) enum SentryCLICommand {
Logs(LogsArgs),
SendMetric(SendMetricArgs),
}
131 changes: 131 additions & 0 deletions src/commands/logs/list.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,131 @@
use anyhow::Result;
use clap::Args;

use crate::api::{Api, Dataset, FetchEventsOptions};
use crate::config::Config;
use crate::utils::formatting::Table;

/// Validate that max_rows is greater than 0
fn validate_max_rows(s: &str) -> Result<usize, String> {
let value = s
.parse::<usize>()
.map_err(|_| "invalid number".to_owned())?;
if value == 0 {
Err("max-rows must be greater than 0".to_owned())
} else {
Ok(value)
}
}

/// Fields to fetch from the logs API
const LOG_FIELDS: &[&str] = &[
"sentry.item_id",
"trace",
"severity",
"timestamp",
"message",
];

/// Arguments for listing logs
#[derive(Args)]
pub(super) struct ListLogsArgs {
#[arg(short = 'o', long = "org")]
#[arg(help = "The organization ID or slug.")]
org: Option<String>,

#[arg(short = 'p', long = "project")]
#[arg(help = "The project ID (slug not supported).")]
project: Option<String>,

#[arg(long = "max-rows", default_value = "100")]
#[arg(value_parser = validate_max_rows)]
#[arg(help = "Maximum number of log entries to fetch and display (max 1000).")]
max_rows: usize,

#[arg(long = "query", default_value = "")]
#[arg(help = "Query to filter logs. Example: \"level:error\"")]
query: String,
}

pub(super) fn execute(args: ListLogsArgs) -> Result<()> {
let config = Config::current();
let (default_org, default_project) = config.get_org_and_project_defaults();

let org = args
.org
.as_ref()
.or(default_org.as_ref())
.ok_or_else(|| {
anyhow::anyhow!("No organization specified. Please specify an organization using the --org argument.")
})?
.to_owned();
let project = args
.project
.as_ref()
.or(default_project.as_ref())
.ok_or_else(|| {
anyhow::anyhow!("No project specified. Use --project or set a default in config.")
})?
.to_owned();

let api = Api::current();

let query = if args.query.is_empty() {
None
} else {
Some(args.query.as_str())
};

execute_single_fetch(&api, &org, &project, query, LOG_FIELDS, &args)
}

fn execute_single_fetch(
api: &Api,
org: &str,
project: &str,
query: Option<&str>,
fields: &[&str],
args: &ListLogsArgs,
) -> Result<()> {
let options = FetchEventsOptions {
dataset: Dataset::OurLogs,
fields,
project_id: Some(project),
cursor: None,
query,
per_page: Some(args.max_rows),
stats_period: Some("1h"),
Copy link
Member

Choose a reason for hiding this comment

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

This value should either be user-configurable, or we should state in the command's --help text that we only display logs from the last hour

Copy link
Member

Choose a reason for hiding this comment

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

So since the way we query the API is descending by time, and we limit to 100 rows via the per_page parameter, we will always get the 100 most recent logs. I did a quick test on our API with 90d and 1h, and the timing is not really different, so I'd propose defaulting to 90d and not making it user-configurable for now, that would always give the user the most recent logs. What do you think about that @szokeasaurusrex?

Copy link
Member

Choose a reason for hiding this comment

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

@shellmayr, is the stats_period even needed in the query string? If not, let's just delete the struct field completely and not send the parameter, assuming that the default time period the server assumes is reasonable. Otherwise, I would just hardcode the 90d default

Copy link
Member

Choose a reason for hiding this comment

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

@szokeasaurusrex I think it defaults to 14 days if not explicitly set

Copy link
Member

Choose a reason for hiding this comment

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

Hmm okay, I guess either the server default or a 90 day default is reasonable in that case

Copy link
Member

Choose a reason for hiding this comment

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

I'd rather it be explicit, so the CLI doesn't rely on current, unspecified behaviour - that way the behaviour should stay the same if something changes in the API configuration.

Copy link
Member

Choose a reason for hiding this comment

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

Very wise

sort: Some("-timestamp"),
};

let logs = api
.authenticated()?
.fetch_organization_events(org, &options)?;

let mut table = Table::new();
table
.title_row()
.add("Item ID")
.add("Timestamp")
.add("Severity")
.add("Message")
.add("Trace");

let logs_to_show = &logs[..args.max_rows.min(logs.len())];
for log in logs_to_show {
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 please rewrite this way, it is easier to read (this is what I meant with my take suggestion in Slack)

Suggested change
let logs_to_show = &logs[..args.max_rows.min(logs.len())];
for log in logs_to_show {
for log in logs.iter().take(args.max_rows) {

let row = table.add_row();
row.add(&log.item_id)
.add(&log.timestamp)
.add(log.severity.as_deref().unwrap_or(""))
.add(log.message.as_deref().unwrap_or(""))
.add(log.trace.as_deref().unwrap_or(""));
}

if table.is_empty() {
println!("No logs found");
} else {
table.print();
}

Ok(())
}
41 changes: 41 additions & 0 deletions src/commands/logs/mod.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
mod list;

use self::list::ListLogsArgs;
use super::derive_parser::{SentryCLI, SentryCLICommand};
use anyhow::Result;
use clap::ArgMatches;
use clap::{Args, Command, Parser as _, Subcommand};

const LIST_ABOUT: &str = "List logs from your organization";

#[derive(Args)]
pub(super) struct LogsArgs {
#[command(subcommand)]
subcommand: LogsSubcommand,
}

#[derive(Subcommand)]
#[command(about = "Manage logs in Sentry")]
#[command(long_about = "Manage and query logs in Sentry. \
This command provides access to log entries.")]
enum LogsSubcommand {
#[command(about = LIST_ABOUT)]
#[command(long_about = format!("{LIST_ABOUT}. \
Query and filter log entries from your Sentry projects. \
Supports filtering by time period, log level, and custom queries."))]
Copy link
Member

Choose a reason for hiding this comment

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

Filtering by time period does not seem to be possible

List(ListLogsArgs),
}

pub(super) fn make_command(command: Command) -> Command {
LogsSubcommand::augment_subcommands(command)
}

pub(super) fn execute(_: &ArgMatches) -> Result<()> {
let SentryCLICommand::Logs(LogsArgs { subcommand }) = SentryCLI::parse().command else {
unreachable!("expected logs subcommand");
};

match subcommand {
LogsSubcommand::List(args) => list::execute(args),
}
}
3 changes: 3 additions & 0 deletions src/commands/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ mod files;
mod info;
mod issues;
mod login;
mod logs;
mod mobile_app;
mod monitors;
mod organizations;
Expand Down Expand Up @@ -57,6 +58,7 @@ macro_rules! each_subcommand {
$mac!(info);
$mac!(issues);
$mac!(login);
$mac!(logs);
#[cfg(feature = "unstable-mobile-app")]
$mac!(mobile_app);
$mac!(monitors);
Expand Down Expand Up @@ -95,6 +97,7 @@ const UPDATE_NAGGER_CMDS: &[&str] = &[
"info",
"issues",
"login",
"logs",
"organizations",
"projects",
"releases",
Expand Down
10 changes: 4 additions & 6 deletions src/commands/send_metric/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,12 +59,10 @@ pub(super) fn make_command(command: Command) -> Command {
}

pub(super) fn execute(_: &ArgMatches) -> Result<()> {
// When adding a new subcommand to the derive_parser SentryCLI, replace the line below with the following:
// let subcommand = match SentryCLI::parse().command {
// SentryCLICommand::SendMetric(SendMetricArgs { subcommand }) => subcommand,
// _ => panic!("expected send-metric subcommand"),
// };
let SentryCLICommand::SendMetric(SendMetricArgs { subcommand }) = SentryCLI::parse().command;
let subcommand = match SentryCLI::parse().command {
SentryCLICommand::SendMetric(SendMetricArgs { subcommand }) => subcommand,
_ => unreachable!("expected send-metric subcommand"),
};

log::warn!("{DEPRECATION_MESSAGE}");

Expand Down
1 change: 1 addition & 0 deletions tests/integration/_cases/help/help-windows.trycmd
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ Commands:
info Print information about the configuration and verify authentication.
issues Manage issues in Sentry.
login Authenticate with the Sentry server.
logs Manage logs in Sentry
monitors Manage cron monitors on Sentry.
organizations Manage organizations on Sentry.
projects Manage projects on Sentry.
Expand Down
1 change: 1 addition & 0 deletions tests/integration/_cases/help/help.trycmd
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ Commands:
info Print information about the configuration and verify authentication.
issues Manage issues in Sentry.
login Authenticate with the Sentry server.
logs Manage logs in Sentry
monitors Manage cron monitors on Sentry.
organizations Manage organizations on Sentry.
projects Manage projects on Sentry.
Expand Down
Loading