Following #111 (comment)

I guess it would imply having a config.local.yaml. But it would be better than having the devtest action used in unit tests in config.nonprod.yaml
Member Author

@grahamalama grahamalama 4 days ago
Yeah, maybe this warrants an issue to decide what we want to do here. Feels like we need to account for our different environments (for development, nonprod, and prod) and decide what will be different about each environment
Member

@bsieber-mozilla bsieber-mozilla 15 hours ago
Alternatively, we opt for one config and make the chosen environments a configuration parameter.

Then the local/nonprod/prod actions would be in a consolidated yaml but enabled/disabled through whichever parameter. (Boolean flag per environment, list of all environments, etc?)

Option #1 : add a local config

• config/config.local.yaml
• config/config.nonprod.yaml
• config/config.prod.yaml

Option #2 : add an environments field to action config

    whiteboard_tag: example
    contact: [email protected]
    description: example configuration
    module: src.jbi.whiteboard_actions.default
    environments: ['nonprod', 'prod']
    parameters:
      jira_project_key: EXMPL

If JBI returns some kind of server error (500 code) to Bugzilla when it tries to deliver a webhook event then Bugzilla keeps retrying that event, delaying delivery of any new events that occur. It has some kind of exponential back-off on retries. After five failures it waits 15 minutes between each attempt. If the webhook fails a certain number of times (100 by default but can be changed in bugzilla's config) then the webhook is just disabled entirely and the owner emailed. In our case I don't think the user's email address is valid so I suspect we just wouldn't hear about it.

The result of this is that if something goes wrong inside JBI, maybe the Jira API is down or something then the event will be retried (this would seem to resolve #33). But if something more permanent goes wrong, say a bug in how we parse the webhook payload or something, then it will completely block syncing.

This seems troublesome. The retrying is good as long as the problem is temporary but I wonder if that is something we should handle internally (as #33 seems to be talking about) rather than completely blocking later events from being processed.

Do we have any monitoring set up that would alert us on server failures?

There was an error spotted by @mixedpuppy on slack here:
https://mozilla.slack.com/archives/C016BTJKM9B/p1645555504432889

After a diagnostic session it was determined that “spaces” (“ “) are causing an error for Jira.

Spaces can be converted to an alternate character (perhaps a . or -) or labels with spaces can be skipped.I’m leaning towards using a . when spaces are found in the tag.

https://mozilla-hub.atlassian.net/jira/software/c/projects/OSS/boards/157/backlog?view=detail&selectedIssue=OSS-474&epics=visible&issueLimit=100&selectedEpic=OSS-388

The '.count' element here appears to be a total count

In #75 we are adding more logic to the default action behavior. Namely setting assignee and status fields.

The chosen approach was to extend the default action behavior using inheritance.

Pros

• No duplicated code between actions

Cons

• Indirection. One has to read and mentally merge two classes in order to understand the final behavior
• Hard to pick (eg. I want status to be set, but not the assignee)

There could be other approaches to consider:

Single action with (many) parameters

    action: src.jbi.whiteboard_actions.default_action
    contact: [[email protected]]
    description: example configuration
    enabled: true
    parameters:
      jira_project_key: EXMPL
      whiteboard_tag: example
      sync_assignee: True
      status_map:
        NEW: "In Progress"
        FIXED: "Closed"

Pros

• A single piece of code to read (default_action.py)

Cons

• The default action code could potentially grow indefinitely, and we would loose the extensibility offered by the action API
• Unit tests have to combine a lot of combinations for the parameters

Support execution of multiple atomic actions

Instead of a single action, define a list of actions to be executed. The specified parameters would be passed to all to simplify configuration.

    contact: [[email protected]]
    description: example configuration
    enabled: true
    actions:
       - src.jbi.whiteboard_actions.create_or_update
       - src.jbi.whiteboard_actions.comments
       - src.jbi.whiteboard_actions.assignee
       - src.jbi.whiteboard_actions.status
    parameters:
      jira_project_key: EXMPL
      whiteboard_tag: example
      status_map:
        NEW: "In Progress"
        FIXED: "Closed"

Pros

• Smaller bits of code
• Modularity
• Easier testing

Cons

• Potential duplication in config if all use-cases want the same set of actions
• Multiple calls to API for separate fields (verbose history in jira?)

There could be other approaches. Maybe the current one is fine, but at least this issue could help us drive a conversation about it (before the code would grow too much)

Otherwise, a session is established on every instantiation. For example GET __heartbeat__ takes more than 5 seconds on each call!

For actions, since the callable (ActionExecutor instance) is cached, it is already indirectly cached

If we have to do many more tests with these services, monkeypatching for each could become cumbersome. Not something to handle in this PR, but it might be worth exploring a dependency injection approach for requiring different services in actions. That way, we could provide some MockBugzillaClient or MockJiraClient to the actions when we test them.

Originally posted by @grahamalama in #31 (comment)

See #42

We are currently in a situation in STAGE where some configured projects are not visible to the Jira client.

I think it's reasonable to receive an alert as soon as possible in that case.

We could have a startup check, but it would be safer to include it in the heartbeat, so that we receive an alert if a project is deleted (without JBI restarting)

We've experienced a few system disruptions caused by Bugzilla disabling the webhook that we've set up to push events to our service.

Is there a way for us to send some request to Bugzilla to check on the status of the webhook? If so, this is something we could potentially incorporate into the healthcheck endpoint.

I'm interesting in being able to measure how much work manual JBI is automating away for us.

I believe the numbers I need for this are:

• number of bugzilla operations done, segmented by type and status (success, failed, etc)
• number of jira operations done, segmented by type and status (success, failed, etc)
• I would also like to sum up those values into a monthly count

Questions:

• how much effort would it take to make that happen?
• any recommendations on different metrics?

Pre-commit and "make lint" (which runs the lint target of the dockerfile) are the two paths in which iSort and Black are used.

The intent is that both paths can continue to use the came configuration files without incurring a circular issue (where Black "solves" an issue, and iSort then views that solution as an issue; and vice versa).

Yeah, not sure what it is--was pulling this from the firestore config; we'll have to reach out to Julien to go through the config together and update this file with contact and description info.

I'll make it an issue to track here.

Originally posted by @bsieber-mozilla in #57 (comment)

There should be a section in README to explicit how to set the application settings

mossop 3 hours ago
I wonder if some kind of no-op action, which didn't need any parameters would be useful for testing

mostlygeek 2 hours ago
or a logger action which just dumps to stdout whatever data it receives ?

From @leplatrem in a Slack DM:

Graham, the v3.1.0 container is broken apparently

docker run mozilla/jira-bugzilla-integration
/opt/pysetup/.venv/bin/python: No module named asgi

Context: https://sentry.io/organizations/mozilla/issues/3416994606/?environment=stage&project=6364263

If the configured Jira user must have the permission to delete issues, then we should add it in the heartbeat check.

Would this be a good usecase for a GunicornSettings(BaseSettings) class to handle picking up env vars, providing defaults, and coercing some variables to ints?

Originally posted by @grahamalama in #1 (comment)

Context: https://sentry.io/organizations/mozilla/issues/3416994548/?environment=stage&project=6364263

This happens in ORJson, introduced in #86

  File "src/app/api.py", line 101, in bugzilla_webhook
    return ORJSONResponse(content=result, status_code=200)
  File "starlette/responses.py", line 49, in __init__
    self.body = self.render(content)
  File "fastapi/responses.py", line 34, in render
    return orjson.dumps(content)

Probably from a Jira response object.

Since our development environment is relatively easy to set up to run tests & linting, and those things are not run through Docker in CI anymore (since #88), we should remove the development stage of the Dockerfile. We should still configure a way to run the production container in "development" mode through environment variables and/or uvicorn.run() params.

pre-commit/pre-commit#1966

When trying to run pre-commit on my local machine something like the above issue kept occurring. I had not modified the files but they kept appearing. Even when allowing them to go through pre-commit and get "fixed" then would fail the next run and then pass again. I'm not sure if it's a mac issue or pre-commit issue.

I had to put it in both locations because iSort ignored the top one.

Originally posted by @bsieber-mozilla in #27 (comment)

See #49 and #48

Looking at JBI logs, it is very hard to debug/troubleshoot a use-case.

When filtering the logs on jsonPayload.Type!="request.summary" or jsonPayload.Type="ignored-requests" we obtain this sort of entry:

{
  "insertId": "1vxv4qutgqc8iupq",
  "jsonPayload": {
    "Fields": {
      "msg": "request: {\"webhook_id\": 22, \"webhook_name\": \"JBI-PROD-InfrastructureAndOperations\", \"event\": {\"action\": \"modify\", \"time\": \"2022-06-30T15:05:07\", \"user\": {\"id\": yyyyyy, \"login\": \"[email protected]\", \"real_name\": \"XXXXX\"}, \"changes\": [{\"field\": \"flag.needinfo\", \"removed\": \"\", \"added\": \"? ([email protected])\"}], \"target\": \"bug\", \"routing_key\": \"bug.modify:flag.needinfo\"}, \"bug\": {\"id\": XXXX, \"is_private\": false, \"type\": \"task\", \"product\": \"Infrastructure & Operations\", \"component\": \"Infrastructure: LDAP\", \"whiteboard\": \"\", \"keywords\": [], \"flags\": [{\"id\": 2114317, \"name\": \"needinfo\", \"requestee\": {\"id\": yyyyy, \"login\": \"xxxxxxxxxx\", \"real_name\": \"XXX XXXX\"}, \"value\": \"?\"}], \"status\": \"ASSIGNED\", \"resolution\": \"\", \"see_also\": [], \"summary\": \"Restore Commit Access (Level 3) for XXXXX\", \"severity\": \"--\", \"priority\": \"\", \"creator\": \"[email protected]\", \"assigned_to\": \"[email protected]\", \"comment\": null}}"
    },
    "Hostname": "jbi-prod-jbi-app-1-6799547446-xlnf4",
    "Severity": 6,
    "Logger": "jbi",
    "Type": "src.jbi.router",
    "EnvVersion": "2.0",
    "Pid": 11,
    "Timestamp": 1656601536174810000
  },
}

I propose that we use structured logs better:

• msg: a short human message
• operation: an enum of predefined actions (eg. create, update, comment, ignore ...)
• action: the matching configured action and its parameters
• requestBody: the input JSON
• responseBody: the output JSON

This way we could track bug numbers better with log queries like:

jsonPayload.Type="ignored-requests"
jsonPayload.Fields.requestBody.bug.id=12345678

Here I posted a patch to phabricator for a bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1780798#c1
JBI commented in the Jira ticket with "None": https://mozilla-hub.atlassian.net/browse/MR2-3072?focusedCommentId=573910

In our Action model, we specify

class Action(YamlModel):
   # ...
   parameters: dict = {}
   # ...

Yet in the Actions model, we have this validation logic:

for name, action in actions.items():
    if name.lower() != action.parameters["whiteboard_tag"]:
        raise ValueError("action name must match whiteboard tag")

This makes the implicit assumption that every Action have a whiteboard_tag.

We should either change our config schema so a whiteboard_tag is a required parameter, or modify this logic somehow.

google-github-actions/setup-gcloud@v0 is not allowed to be used in mozilla/jira-bugzilla-integration. Actions in this workflow must be: within a repository that belongs to your Enterprise account, created by GitHub or match the following: !/mozilla/**, !mozilla/**, ./**, 10up/wpcs-action@*, aws-actions/*, docker/*, pypa/[email protected], slackapi/slack-github-action@*, codecov/codecov-action@v2.

EDIT:
Link to bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1759241

The following lines establish defaults for a a few of the action config fields:

Perhaps a new section in the config.*.yaml could be added to control this from configuration.

Should there be a new section? Maybe there's an alternate route to explore here

Should this new section be: default, default.actions, actions._default, or actions.default.

• default: as a top-level section
• default.actions: a top-level section that has a section dedicated to the other top-level section action
• actions._default: adding the underscore should ensure yamllint keeps this element at the top of the list (we can also remove ordering of keys from yamllint config)
• actions.default 🤷

Context: Feedback from the ITEO program review

• metrics for each configured action about how many syncs it sent from BZ to Jira
• can go to grafana
• gives us an idea of how much manual work JBI has saved / automated

make test fails with:

Creating jira-bugzilla-integration_web_run ... done
/docker-entrypoint.sh: 15: exec: bin/test.sh: Permission denied
ERROR: 126
make: *** [test] Error 126

The current user id is not passed as build arg (id -u), and the host user id does not match the container app id (10001).

As called out in #3 (comment)

These messages can be useful for developers looking to delve into the code.

• C0114 - enabled
• C0115 - enabled
• C0116 - enabled

When pairing with @leplatrem yesterday, we noticed a few things about our current logging output:

• Exceptions weren't being logged to stdout
• the root logger was seemingly overwritten by some other logger

For this issue, we should investigate our logging configuration and ensure that:

• all relevant logs are emitted
• all logs are handled by dockerflow.logging.JsonLogFormatter
• access logs are disabled
• there are no duplicate logs

Modify workflow from GH to Circle

Since src.app.api has a main, we should be able to run the app locally...

$ JIRA_USERNAME=foo JIRA_API_KEY=bar BUGZILLA_API_KEY=bz PORT=8000 poetry run python -m src.app.api

Traceback (most recent call last):
  File "/opt/homebrew/Cellar/[email protected]/3.9.13_1/Frameworks/Python.framework/Versions/3.9/lib/python3.9/runpy.py", line 197, in _run_module_as_main
    return _run_code(code, main_globals, None,
  File "/opt/homebrew/Cellar/[email protected]/3.9.13_1/Frameworks/Python.framework/Versions/3.9/lib/python3.9/runpy.py", line 87, in _run_code
    exec(code, run_globals)
  File "/Users/mathieu/Code/Mozilla/jira-bugzilla-integration/src/app/api.py", line 143, in <module>
    uvicorn.run(
  File "/Users/mathieu/Library/Caches/pypoetry/virtualenvs/jira-bugzilla-integration-fgdRkTUv-py3.9/lib/python3.9/site-packages/uvicorn/main.py", line 457, in run
    sock = config.bind_socket()
  File "/Users/mathieu/Library/Caches/pypoetry/virtualenvs/jira-bugzilla-integration-fgdRkTUv-py3.9/lib/python3.9/site-packages/uvicorn/config.py", line 551, in bind_socket
    sock.bind((self.host, self.port))
TypeError: an integer is required (got type str)

It looks like environment.py does not have the right types.

I also tried to run the app locally through Gunicorn.

$ poetry run gunicorn -k uvicorn.workers.UvicornWorker -c bin/gunicorn_conf.py src.app.api:app
...
  File "/Users/mathieu/Library/Caches/pypoetry/virtualenvs/jira-bugzilla-integration-fgdRkTUv-py3.9/lib/python3.9/site-packages/gunicorn/workers/workertmp.py", line 22, in __init__
    raise RuntimeError("%s doesn't exist. Can't create workertmp." % fdir)
RuntimeError: /dev/shm doesn't exist. Can't create workertmp.

After a quick fix in gunicorn_conf.py to remove the hard-coded value, I could get Gunicorn to work locally:

$ JIRA_USERNAME=foo JIRA_API_KEY=bar BUGZILLA_API_KEY=bz WORKER_TMP_DIR=/tmp/ poetry run gunicorn -k uvicorn.workers.UvicornWorker -c bin/gunicorn_conf.py src.app.api:app
..
[2022-07-13 15:42:12 +0200] [28107] [INFO] Starting gunicorn 20.1.0
[2022-07-13 15:42:12 +0200] [28107] [INFO] Listening at: http://0.0.0.0:8000 (28107)
[2022-07-13 15:42:12 +0200] [28107] [INFO] Using worker: uvicorn.workers.UvicornWorker
[2022-07-13 15:42:12 +0200] [28109] [INFO] Booting worker with pid: 28109
[2022-07-13 15:42:12 +0200] [28110] [INFO] Booting worker with pid: 28110
...

So, should we keep if __name__ == "__main__" block in src/app/api.py and its related settings in src/environment.py?

We saw odd serialization behavior in #136 and #137 that we closed with #139 and #142.

We only verified our fixes with manual testing though -- ideally this would be covered by a test case or two.

We should also investigate why the model-level exclude config didn't seem to have the desired outcome (we had to use the dict(exclude={...} method)

Similar issue to: lincolnloop/goodconf#8

Tracking this issue in bugzilla: https://bugzilla.mozilla.org/show_bug.cgi?id=1752191

https://sentry.io/organizations/mozilla/issues/3438704216/

Design Endpoint/API that can replace generate-helper.sh?

• Is this worth it?

HTML Jinja2 Template for Powered by JBI

What happens here if for some reason, jira_client.issue_add_comment fails -- maybe there's some blip of a 500 error on Jira's end or something. Would we be in a state where only some of the comments we want on the Jira ticket appear? Should we have some sort of retry mechanism?

Originally posted by @grahamalama in #31 (comment)

This way we would be alerted when the Bugzilla Webhook gets disabled (see #181 )

When we enabled this in prod, Bugzilla returned 429 Client Error: Too Many Requests for url: https://bugzilla.mozilla.org/xmlrpc.cgi.

• sentry report

Questions

• does JBI recover from these gracefully?
• are these queued somewhere for retry?

We want to report stacktraces to Sentry.

Configure Sentry from a SENTRY_DSN env var. Skip if empty.

• Count accepted / rejected BZ events
• Measure API calls latency
• Measure execution time

Configure client from STATSD_DSN env var. Skip if empty.

    info = {}
    version_path = Path(__file__).parents[2] / "version.json"
    if version_path.exists():
        info = json.loads(version_path.read_text(encoding="utf8"))

Don't know if I got that quite right, but the general comment is that Pathlib might be nice here

Originally posted by @grahamalama in #1 (comment)

make lint 3.54s user 1.93s system 10% cpu 51.279 total

Shouldn't we run pre-commit run --all-files instead ? (98.66s user 5.41s system 522% cpu 19.920 total)

Slack reference

From the FastAPI docs:

If you have a cluster of machines with Kubernetes (...) then you will probably want to handle replication at the cluster level instead of using a process manager (like Gunicorn with workers) in each container.
...
In those cases, you would probably want to (...) [run a] single Uvicorn process instead of running something like Gunicorn with Uvicorn workers.

I think this means we should:

• change the default container CMD to run uvicorn
• remove gunicorn_conf.py