Skip to content
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

ADF pipeline in toolkit v0.2 breaks due to syntax errors in set scope and set date step #557

Closed
DUeffing opened this issue Jan 23, 2024 · 18 comments · Fixed by #574
Closed
Assignees
Labels
Needs: Attention 👋 Issue or PR needs to be reviewed by the author or it will be closed due to no activity Skill: Data factory Data Factory integration Status: 📋 Pending confirmation Waiting on explicit confirmation that the issue was resolved in previous release Tool: FinOps hubs Data pipeline solution Type: Bug 🐛 Something isn't working
Milestone

Comments

@DUeffing
Copy link

🐛 Problem

ADF pipeline "msexports_ETL_ingestion" the steps "Set scope" & "Set date" outlining the following syntax error: Syntax error: Missing period. This breaks the pipeline execution

👣 Repro steps

open the pipeline, select the settings tab, the value is marked red

🤔 Expected

execution of the step in the pipeline succeeds

📷 Screenshots

image

🙋‍♀️ Ask for the community

the line of code looks like the following:
@replace(split(pipeline().parameters.folderName,variables('folderArray')[sub(length(variables('folderArray')), if(greater(length(variables('folderArray')[sub(length(variables('folderArray')), 2)]), 12), 3, 4))])[0],'msexports','ingestion')

error: Position 196 Syntax error: Missing period
I assume that their should be a period between ('folderArry') and [sub(length.... however this even not work

We could use your help:

  1. Please vote this issue up (👍) to prioritize it.
  2. Leave comments to help us solidify the vision.
@DUeffing DUeffing added Needs: Triage 🔍 Untriaged issue needs to be reviewed Type: Bug 🐛 Something isn't working labels Jan 23, 2024
Copy link

Uh oh! @DUeffing, the image you shared is missing helpful alt text. Check your issue body.

Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image.

Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs.

@DUeffing DUeffing changed the title ADF pipeline in toolkit v0.2 breaks to due syntax error in set scope and set date step ADF pipeline in toolkit v0.2 breaks due ot syntax error in set scope and set date step Jan 23, 2024
Copy link

Uh oh! @DUeffing, the image you shared is missing helpful alt text. Check your issue body.

Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image.

Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs.

@DUeffing DUeffing changed the title ADF pipeline in toolkit v0.2 breaks due ot syntax error in set scope and set date step ADF pipeline in toolkit v0.2 breaks due to syntax errors in set scope and set date step Jan 23, 2024
Copy link

Uh oh! @DUeffing, the image you shared is missing helpful alt text. Check your issue body.

Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image.

Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs.

@flanakin
Copy link
Collaborator

@DUeffing I saw the same error in the ADF but I wasn't able to pinpoint the actual issue and everything was working as expected. I assumed it was a bug in ADF.

@MSBrett do you see anything?

@flanakin flanakin added Skill: Data factory Data Factory integration Tool: FinOps hubs Data pipeline solution Status: 🕵️‍♀️ Investigating Issue is actively being investigated labels Jan 25, 2024
@DUeffing
Copy link
Author

@flanakin thanks for the feedback, for the sack of clarity. I used the following source for the deployment:

https://github.com/microsoft/finops-toolkit/releases/tag/v0.2 and insight of it the [finops-hub-v0.2.zip] file.

@flanakin
Copy link
Collaborator

flanakin commented Jan 26, 2024

Thanks for confirming. This code is new in 0.2, so that's why I didn't ask 🙂

My suspicion is that it's a bug in their parser with variables. But that's just a guess. We could remove the variable. I added that hoping it would make the code more readable. It's a lot better, but these expressions are always hard to read. They remind me of when people say regular expressions are "write once, read never" 🙃

@DUeffing
Copy link
Author

thanks for the feedback and I used copilot for explaining me what the code is doing and as well how to fix it. The explanation was good, but the proposal to fix it not. Nevertheless, the variables must be set otherwise the destination path for the parquet file will not set correct which breaks the entire pipeline. Any ideas how to fix it? Otherwise I would switch back to the 0.1 version of the pipeline as this worked as well with FOCUS data.

@erincon01
Copy link

I see the same error in v0.2.

In the set Date there is a similar error:
@substring(variables('folderArray')[sub(length(variables('folderArray')), if(greater(length(variables('folderArray')[sub(length(variables('folderArray')), 2)]), 12), 2, 3))], 0, 6)

I suggest use the set options I mention here instead of using the folderArray variable:
#523

@flanakin
Copy link
Collaborator

@DUeffing wait, is it breaking for you? I see the error in the ADF portal, but the code runs fine. Are you saying it's failing for you?

@DUeffing
Copy link
Author

DUeffing commented Jan 29, 2024

Hi @flanakin , I tested it again and the pipeline succeeded. However, I recognized that the event grid subscriber does not exists anymore and therefore the pipeline does not starts automatically. I could only start the pipeline by providing the values for the folder and the file manually in ADF for the trigger.

btw, I redeployed the the hub entirely from scratch before the new test. And it would be great if you can provide an example what the expected folder structure within the msexports container should look like

@erincon01
Copy link

@DUeffing , this is the folder structure I have:

keep in mind that the mxexport folder tends to be empty once the file is converted to .parquet.

image

@t-esslinger
Copy link

t-esslinger commented Jan 30, 2024

I recognized the same issue with the syntax error. In my case the pipeline is running fine but the trigger has not been active after deployment:
image

@DUeffing : You might want to check that in ADF under Manage -> Author section -> Triggers

@DUeffing
Copy link
Author

Hi, the trigger is out of the box deactivated / stopped obviously. When activating the trigger, it requires to publish the change to the pipeline for getting effective. Which then in turn would create the corresponding event grid subscription (as an requirement for the trigger itself). However, this is not working as the publishing failed due to the syntax error described beforehand

image

@flanakin any thoughts on this one?

@flanakin flanakin removed the Needs: Triage 🔍 Untriaged issue needs to be reviewed label Jan 30, 2024
@t-esslinger
Copy link

Sorry, wasn't sure if you were aware of the deactivated trigger. Imo in version 0.1 the trigger was active by default.

@thecloudman
Copy link

Any update on when the ADF pipelines will be fixed. I redeployed yesterday and still the same issue.
image

@flanakin flanakin added this to the 0.2.1 milestone Feb 1, 2024
@flanakin
Copy link
Collaborator

flanakin commented Feb 1, 2024

Here's a release candidate: https://gist.github.com/flanakin/419b5ed778d12a109b297d0dd406c10f
I did a quick pass to make sure the files land in ingestion. Will confirm the rest later.

I moved the ADF code around so the error isn't there anymore. Still not sure if it was a real bug or not.

I did find a bug that was causing the trigger to not get started. That's also fixed.

@DUeffing
Copy link
Author

DUeffing commented Feb 2, 2024

thanks @flanakin, will test it accordingly and let you know

@flanakin flanakin linked a pull request Feb 3, 2024 that will close this issue
@flanakin
Copy link
Collaborator

flanakin commented Feb 3, 2024

@DUeffing Made some updates to fix a path issue and validated using the Cost summary report. Please let me know if you run into any issues.

https://aka.ms/finops/hubs/rc

@flanakin flanakin removed their assignment Feb 3, 2024
@flanakin flanakin added Status: 📋 Pending confirmation Waiting on explicit confirmation that the issue was resolved in previous release Needs: Attention 👋 Issue or PR needs to be reviewed by the author or it will be closed due to no activity and removed Status: 🕵️‍♀️ Investigating Issue is actively being investigated labels Feb 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs: Attention 👋 Issue or PR needs to be reviewed by the author or it will be closed due to no activity Skill: Data factory Data Factory integration Status: 📋 Pending confirmation Waiting on explicit confirmation that the issue was resolved in previous release Tool: FinOps hubs Data pipeline solution Type: Bug 🐛 Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants