[VDP-36907] Label Microsoft Fabric requests #2
Conversation
🎉 Snyk checks have passed. No issues have been found so far.✅ code/snyk check is complete. No issues have been found. (View Details) |
vepi/vena_etl.py
Outdated
| try: | ||
| from notebookutils import mssparkutils | ||
| return f"{lib_name}/{lib_version}; platform/Microsoft Fabric" | ||
| except ImportError: | ||
| return f"{lib_name}/{lib_version}" |
There was a problem hiding this comment.
This library notebookutils / mssparkutils only exists in Microsoft Fabric, so it will fail outside of that environment. I'm open to other suggestions for identifying Fabric but many of the other approaches required more dependencies like os and the code looked more suspect.
There was a problem hiding this comment.
Might be able to use the os object (import os) to determine where the script is running. If you print the environment variables inside a fabric notebook, you'll see that there's some references to "fabric", "azure" and "microsoft".
There was a problem hiding this comment.
Yeah I tried the import os approach and the code worked but was messy, I had to check for the AZURE_OPENAI_API_KEY key which had this in it "place_holder_for_fabric_internal"
There was a problem hiding this comment.
Maybe we can ask microsoft to just place an environment variable that clearly states that python is running in fabric. If they add it then we could come back to this later and update it
| api_user (str): API user from Vena authentication token | ||
| api_key (str): API key from Vena authentication token | ||
| template_id (str): ETL template ID | ||
| data_source (str): Please indicate the ERP or data source, this helps Vena with support and troubleshooting.(e.g., NetSuite, Dynamics 365, SAP, ADP, Other) |
There was a problem hiding this comment.
How do you feel about making a bunch of string constants or making this an enum? Standardizing these values would make triaging and gathering metrics easier. For example, one customer might use intacct and another might use sage but they're both the same source
There was a problem hiding this comment.
Only issue with this is if we don't have an Enum for the all the different systems(which there are too many to list). This will be used to help us understand issues with the library or import jobs in Vena.
There was a problem hiding this comment.
Yea I was thinking we do something like
enum DataSources {
NETSUITE,
INTACCT,
// If your source is not listed here then add it
}
This way we standardize the most common sources and then in future updates we can extend this to include the common enums that users create themselves
There was a problem hiding this comment.
Oh did you mean we should recommend sources names(with the ENUM), but don't add them to Repo as mandatory, so its still a string?
There was a problem hiding this comment.
that works too. Just want to give the customer a list of standardized names, whether that's through an enum, list of string constants or some other way
There was a problem hiding this comment.
Ok, I think I'll update some of the sample code in the ReadMe with the most common ones in a follow up PR. I don't want it to be strict, and tbh some users won't want to share their data source, which is fine.
MxSahil
left a comment
MxSahil
left a comment
There was a problem hiding this comment.
Left a suggestion for the data_sources variable but it LGTM
For Vena Developers
Ticket #: https://venasolutions.atlassian.net/browse/VDP-36907
Description
Override the user agent to be able to identify requests used in Microsoft Fabric
Type of Change
Checklist
Additional Notes
None