-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
chore: renamed non-inclusive language #18319
Changes from 1 commit
9b69aeb
2ef228c
a9973b7
119900b
083ba77
054f820
d24c29f
03842a3
5c8c545
e5db976
fcd6346
d069d2f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
name: Inclusive Language Linter | ||
|
||
on: [pull_request] | ||
|
||
jobs: | ||
lint: | ||
name: Linting language | ||
runs-on: ubuntu-latest | ||
steps: | ||
- uses: actions/checkout@v2 | ||
- uses: get-woke/woke-action-reviewdog@v0 | ||
with: | ||
github-token: ${{ secrets.GITHUB_TOKEN }} | ||
reporter: github-pr-review | ||
level: warning |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
.wokeignore | ||
posthog/management/sql/0050_event_partitions.sql | ||
posthog/migrations/0001_initial_squashed_0284_improved_caching_state_idx.py | ||
posthog/migrations/0160_organization_domain_whitelist.py | ||
posthog/migrations/0161_property_defs_search.py | ||
posthog/migrations/0223_organizationdomain.py | ||
*.ambr | ||
frontend/src/scenes/plugins/source/types/packages.json | ||
# External lib settings | ||
posthog/settings/web.py | ||
# External lib settings | ||
ee/settings.py | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -71,7 +71,7 @@ export class EventPipelineRunner { | |
this.originalEvent = event | ||
} | ||
|
||
isEventBlacklisted(event: PipelineEvent): boolean { | ||
isEventDisallowed(event: PipelineEvent): boolean { | ||
// During incidents we can use the the env DROP_EVENTS_BY_TOKEN_DISTINCT_ID | ||
// to drop events here before processing them which would allow us to catch up | ||
const key = event.token || event.team_id?.toString() | ||
|
@@ -87,14 +87,14 @@ export class EventPipelineRunner { | |
this.hub.statsd?.increment('kafka_queue.event_pipeline.start', { pipeline: 'event' }) | ||
|
||
try { | ||
if (this.isEventBlacklisted(event)) { | ||
if (this.isEventDisallowed(event)) { | ||
eventDroppedCounter | ||
.labels({ | ||
event_type: 'analytics', | ||
drop_cause: 'blacklisted', | ||
drop_cause: 'disallowed', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could this impact analytics that are relied on? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great question. @tiina303 Git blame tells me you were the last around these parts of the code. Do you know whether there are any implications for changing this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ooh indeed great catch, it is changing the metric label, so yes filtering for it in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All good with this one. For any metrics I'd just search in the charts repo, where we have dashboards/alerts https://github.com/search?q=repo%3APostHog%2Fcharts%20blacklisted&type=code There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, I'm rather new with Grafana and I'm not too sure what I'm looking for in there. Considering the search in the charts repo returns nothing for me, are we all good, or are those two things not connected? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes the search returns nothing (for neither repo), so we don't use the label explicitly in alerts nor dashboards. |
||
}) | ||
.inc() | ||
return this.registerLastStep('eventBlacklistingStep', null, [event]) | ||
return this.registerLastStep('eventDisallowedStep', null, [event]) | ||
} | ||
let result: EventPipelineResult | ||
const eventWithTeam = await this.runStep(populateTeamDataStep, [this, event], event.team_id || -1) | ||
|
@@ -204,7 +204,7 @@ export class EventPipelineRunner { | |
// for a reason that we control and that is transient. | ||
return true | ||
} | ||
// TODO: Blacklist via env of errors we're going to put into DLQ instead of taking Kafka lag | ||
// TODO: Disalow via env of errors we're going to put into DLQ instead of taking Kafka lag | ||
Gilbert09 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return false | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm planning on keeping this in for the time being, it makes checking the code base for any future regressions much easier for me