Skip to content

Conversation

@MeredithAnya
Copy link
Member

@MeredithAnya MeredithAnya commented Oct 14, 2020

Context:
Ecosystem is working on making stack trace linking a core piece of the repository provider integrations (GH, GitLab, etc). This means taking someone from a file in the Sentry stack trace to a file in their source code. There are challenges with finding out exactly where to link someone:

  1. Integrations and repositories are on the organization level.
    • This makes it hard to go from event stack trace in a project to the correct repository if you have many repos that are connected to multiple projects.
  2. The file path in the stack trace may not be formatting quite right to link correctly to the source code - some additional formatting may be needed on a case to case basis.
  3. The code may have changed since the error in Sentry was captured.
    • We can try to find the latest commit sha in order to redirect the user to the correct version of the code, but if we can't we may need a default branch to fall back on.

This why we've decided to have a configuration required, the configuration may change over time as we iterate, so the explanation in this PR description is may be outdated later.

Configuration Fields:

project, repository

  • I think the project and repository are self explanatory, the repository has the integration_id and we can get an organization_integration from the project.organization and the integration.

organization_integration

  • We actually need this to be able to get the configurations easily when we load the integration configuration page

input_path, output_path

  • If needed, the input_path is the part of the stack trace file path that needs to be replaced with a different value, which is the output_path. If the file doesn't need re-formatting, both the input and output paths can be empty strings.

default_branch

  • If we can't find the version of the code for the file path, we may need a default branch. Some providers may handle this, but others may still require that default be provided in the url.

@MeredithAnya MeredithAnya requested a review from a team October 14, 2020 19:26
@MeredithAnya MeredithAnya requested a review from a team as a code owner October 14, 2020 19:26
@github-actions
Copy link
Contributor

github-actions bot commented Oct 14, 2020

This PR has a migration; here is the generated SQL

BEGIN;
--
-- Create model RepositoryProjectPathConfig
--
CREATE TABLE "sentry_repositoryprojectpathconfig" ("id" bigserial NOT NULL PRIMARY KEY, "date_updated" timestamp with time zone NOT NULL, "date_added" timestamp with time zone NULL, "stack_root" text NOT NULL, "source_root" text NOT NULL, "default_branch" text NULL, "organization_integration_id" bigint NOT NULL, "project_id" bigint NOT NULL, "repository_id" bigint NOT NULL);
--
-- Alter unique_together for repositoryprojectpathconfig (1 constraint(s))
--
ALTER TABLE "sentry_repositoryprojectpathconfig" ADD CONSTRAINT "sentry_repositoryproject_project_id_stack_root_e376b891_uniq" UNIQUE ("project_id", "stack_root");
ALTER TABLE "sentry_repositoryprojectpathconfig" ADD CONSTRAINT "sentry_repositorypro_organization_integra_d70daa81_fk_sentry_or" FOREIGN KEY ("organization_integration_id") REFERENCES "sentry_organizationintegration" ("id") DEFERRABLE INITIALLY DEFERRED;
ALTER TABLE "sentry_repositoryprojectpathconfig" ADD CONSTRAINT "sentry_repositorypro_repository_id_e5400943_fk_sentry_re" FOREIGN KEY ("repository_id") REFERENCES "sentry_repository" ("id") DEFERRABLE INITIALLY DEFERRED;
CREATE INDEX "sentry_repositoryprojectpa_organization_integration_i_d70daa81" ON "sentry_repositoryprojectpathconfig" ("organization_integration_id");
CREATE INDEX "sentry_repositoryprojectpathconfig_project_id_0de53adf" ON "sentry_repositoryprojectpathconfig" ("project_id");
CREATE INDEX "sentry_repositoryprojectpathconfig_repository_id_e5400943" ON "sentry_repositoryprojectpathconfig" ("repository_id");
COMMIT;

__core__ = False

repository = FlexibleForeignKey("sentry.Repository")
project = FlexibleForeignKey("sentry.Project", db_constraint=False)
Copy link
Member Author

Choose a reason for hiding this comment

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

@wedamija I wasn't sure if I should have added db_index=False here if the unique constraint will also create an index.

Copy link
Member

Choose a reason for hiding this comment

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

You can, it's probably fine either way. That's really just an optimisation, and this table probably won't be large enough or high volume enough for it to be important.

class RepositoryProjectPathConfig(DefaultFieldsModel):
__core__ = False

repository = FlexibleForeignKey("sentry.Repository")
Copy link
Contributor

Choose a reason for hiding this comment

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

@MeredithAnya should we have db_constraint=False here as well?

Copy link
Member

Choose a reason for hiding this comment

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

It's probably fine, I don't think repository is very high volume. Can't check at the moment :(

Copy link
Member

@wedamija wedamija left a comment

Choose a reason for hiding this comment

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

Migration looks good to me

__core__ = False

repository = FlexibleForeignKey("sentry.Repository")
project = FlexibleForeignKey("sentry.Project", db_constraint=False)
Copy link
Member

Choose a reason for hiding this comment

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

You can, it's probably fine either way. That's really just an optimisation, and this table probably won't be large enough or high volume enough for it to be important.

class RepositoryProjectPathConfig(DefaultFieldsModel):
__core__ = False

repository = FlexibleForeignKey("sentry.Repository")
Copy link
Member

Choose a reason for hiding this comment

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

It's probably fine, I don't think repository is very high volume. Can't check at the moment :(

Comment on lines 42 to 43
('input_path', models.TextField()),
('output_path', models.TextField()),
Copy link
Member

Choose a reason for hiding this comment

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

stack_path and repository_path might be clearer names for how these columns would be used.

Copy link
Member Author

Choose a reason for hiding this comment

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

@scefali what are your thoughts? I like these names

Copy link
Contributor

Choose a reason for hiding this comment

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

@MeredithAnya maybe base_stack_path and base_repository_path might make more sense

Copy link
Member Author

@MeredithAnya MeredithAnya Oct 15, 2020

Choose a reason for hiding this comment

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

what about stack_root, source_root? @scefali I actually think using repo might seem like it should include the repo as part of the path

Copy link
Contributor

Choose a reason for hiding this comment

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

@MeredithAnya sounds good to me

Copy link
Member

Choose a reason for hiding this comment

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

stack_root an source_root are 🌈

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.

5 participants