fix(ingest/powerbi): use dataset workspace id as key for parent container

[looppi]

In our production environment, I noticed that all the datasets were under a single workspace container, which was not the case. On closer inspection, I found out that if there are entities from multiple workspaces in a single "batch", then all the datasets would get the last workspace key reference which was processed.

Here, I created a very basic workaround for this issue, piggybacking the workspace reference from the moment, when it was added to the processed_datasets set and using that reference when making connection between workspace container and dataset.

The problem happens only when both extract_datasets_to_containers and extract_workspaces_to_containers are enabled. As if one of the features is disabled, there's no relation between the two containers. The iteration here emitted wrong workspace references to the dataset containers as it used self.workspace_key instead of a key which matched the datasets origin workspace.

Checklist

• The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
• Links to related issues (if applicable)
• Tests for the changes have been added/updated (if applicable)
• Docs related to the changes have been added/updated (if applicable). If a new feature has been added a Usage Guide has been added for the same.
• For any breaking change/potential downtime/deprecation/big changes an entry has been made in Updating DataHub

[looppi]

I managed to reproduce the issue in the test case by allowing multiple workspaces to be scanned at test_workspace_container. The bug is that the bad container references are emitted in the end of ingestion.

[looppi]

I just noticed that this "fix" is going to introduce another bug. I think we need to discuss further how to proceed.

Problem is that Workspace container generation doesn't use platform_instance in URN generation, but my fix does use it. This fix will result in broken container references if platform_instance is configured.

I'm going to try to figure out a way, where there would be only one place for workspace container key generation and it would also fix the issue of the workspace keys getting mixed.

In every case, changing the workspace key generation would be major as it creates new resources for every new user.

[hsheth2]

@looppi honestly having a bit of trouble following the issue explanation. It kinda sounds like we previously weren't respecting the platform_instance value set in the config when generating workspace container urns, and that sounds like a bug. I'd be ok with a breaking change to powerbi container urns, so long the change doesn't have any impact if platform_instance was not set in the config (which should be the standard behavior for our urn guid generation).

Also, it feels like the powerbi code is more complex than it needs to be, but that's a separate thing

[looppi]

I found it quite difficult to describe the issue. You guessed correctly, the previous implementation wasn't respecting platform_instance in every use case. Previously the platform_instance was used when generating references to the container, but not when actually creating the container.

I made the change in this PR to respect the platform_instance value in every case.

I actually found a more elegant way to get rid of the "workspace reference leakage" issue, for which I originally created this PR. Just set the processed_datasets to empty set every time workspace changes. The implementation already runs the code for every workspace so this might have been just an oversight from earlier changes.

[looppi]

This was missing, which meant that if platform_instance was set, the actual container metadata had a different urn as the references which were supposed to point to the same container.

[looppi]

Work around the leakage of processed datasets over workspaces by setting the processed_datasets empty for every single workspace instance.

[hsheth2]

This code mostly looks good, and is a lot cleaner

One thing that I just noticed - we make the assumption that parent container are emitted before their children in a few places downstream (particularly when generating "browse paths v2" for the containers). In effect, that makes us generate browsePathV2 aspects incorrectly if you're using both extract_workspaces_to_containers and extract_datasets_to_containers (both the workspace and the dataset should show up in the browse path v2, but in the golden file here only the dataset shows up)

I'd still be happy to accept this PR as-is because it's still a net improvement, but I was wondering if this is something you'd like to take a stab at

[looppi]

I'll try to do this, I'll get back to you when I have something to show!

[looppi]

This is kind of ugly, but I couldn't find any other implementation for gen_containers and decided that for this implementation it's easier to unwrap the metadata from the work unit rather than create another implementation for container creation.

[hsheth2]

this is fine - we should probably make gen_containers return MCPs instead of workunits

[hsheth2]

LGTM!

[hsheth2]

this is fine - we should probably make gen_containers return MCPs instead of workunits