Page MenuHomeElementl

dagit_base_url instance setting
AbandonedPublic

Authored by sandyryza on Jun 24 2021, 11:18 PM.

Details

Summary

This allows hooks and failure sensors to construct links to the right instance dagits.

Test Plan

bk

Diff Detail

Repository
R1 dagster
Branch
instance-dagit-url (branched from master)
Lint
Lint Passed
Unit
No Test Coverage

Event Timeline

My initial thought was for the instance to get the domain without explicitly asking users to configure. But I'm less familiar with the dagit server, so am not sure if it's feasible.

Besides, could there be multiple dagit servers pointing to the same instance? My mental model is dagit points to an instance as opposed to this diff which is more of an instance points to a dagit base url.

Read only dagit would be an example of a second dagit (with a new url) pointed at the same instance

where do the links appear - are they within dagit, and are they to specific entities (e.g. a specific pipeline or sensor)? I'm wondering if this is a situation where some kind of descriptor of an object should be stored instead (and then the problem of turning that object into a URL is separate) but I think i am missing some context

@dgibson for example, a pipeline failed and you get a slack alert which includes a dagit link. It can quickly navigate you back to the run page, e.g. "Go To Pipeline Run" button:

image.png (318×1 px, 40 KB)

a pipeline failed and you get a slack alert which includes a dagit link.

How bad is it to pass the expected domain (and optional path prefix) at the call-site that forms the the slack alert body?

python_modules/dagster/dagster_tests/core_tests/instance_tests/test_instance.py
259–261

theoretically needs to respect path_prefix too which is currently a cli flag to dagit

How bad is it to pass the expected domain (and optional path prefix) at the call-site that forms the the slack alert body?

this is how the slack_on_failure is currently implemented.

things could get awkward when you want to reuse the same code in different modes - different modes (e.g. local, dev, prod) usually mean multiple dagit instances

  • in the hook case, it's not too bad. bc it can use resources, you can just have a "global config resource" with expected domain url for different modes, like:
@success_hook(required_resource_keys={"slack", "base_url"})
def slack_on_success(context: HookContext):
    run_page_url = f"{context.resources.base_url}/instance/runs/{context.run_id}?logs=query%3A{context.step_key}&selection={context.step_key}"
    ...
  • but it gets awkward in sensors because resource isn't available, so you probably need to encode a mode-to-dagit-url logic in your sensor code. for example:
if run.mode == "prod":
    base_url = "http://prod.dagit"
elif run.mode == "staging_live_data":
    base_url = "http://dev.dagit"
else:
    base_url = "http://localhost:3000"

My initial thought was for the instance to get the domain without explicitly asking users to configure.

I think, in general, you can't figure out the domain in this way. I.e. a user might want to host dagit via a proxy, and dagit doesn't necessarily know about the proxy.

How bad is it to pass the expected domain (and optional path prefix) at the call-site that forms the the slack alert body?

I guess there's the question of how the caller would figure out what to pass. Maybe the convention would be to define a module with a bunch of global variables like PROD_DAGIT_INSTANCE_URL? Would that be better?

I guess there's the question of how the caller would figure out what to pass. Maybe the convention would be to define a module with a bunch of global variables like PROD_DAGIT_INSTANCE_URL? Would that be better?

My concerns around this are what a user would expect this setting to do and interactions with things like the --path-prefix flag to the dagit cli. In that light, I think something like using a global variable is more likely to establish the right expectations that this is a manually declared assumption that is not verified and not updated with any other information that dagster may have seen.

johann added a subscriber: johann.
dgibson requested changes to this revision.Jul 1 2021, 1:53 PM

not totally clear where this landed, but it does feel off to put this as a new instance field to me - it makes it seem like it will configure something or *determine* the base URL for dagit / actually change behavior as opposed to being an assumption that's actually set elsewhere (kind of just repeating what alex said I guess)

This revision now requires changes to proceed.Jul 1 2021, 1:53 PM

If we don't merge something like this, I think the path to supporting it would basically be to expect users to have two sensors: https://dagster.phacility.com/D8652. @yuhan does that sound reasonable to you?