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

adding support for --includeTags option #108

Merged
merged 1 commit into from
Feb 16, 2024
Merged

Conversation

erincon01
Copy link
Contributor

Hello @mivano

I need to export the daily costs data by resourceId and include into the resultset the resource Tags.
It is useful for scenarios where data is exported to analyze using Power BI.

I have changed the code to support exporting Tags when:

  • Output format is json, jsonc, and csv.
  • Report type is DailyCost.

I have chaged the API version to 2023-03-01.

This is the logic of the new option:

  • if --includeTags is added into the call, a new column called Tags will include the tags for the dimension.
  • The new column type is Json.
  • To avoid breaking things: if the request is not DailyCost, is not Json, is not JsonC, and is not Csv, the includeTags variable is set to false.

I had to add a new record called CostDailyItemWithoutTags where rows were moved when includeTags is False.
It was a small switch in the Csv Output formaters depending on the setgings:

image

For the Json formater was easier:

image

I have edited the readme.md to include usage description.

I am happy you consider adding this option to the main brach!

Regards,
Eladio

Copy link

welcome bot commented Feb 14, 2024

Thanks for opening this pull request!

@mivano
Copy link
Owner

mivano commented Feb 14, 2024

Thanks for the PR, I will have a look and get back to you!

@erincon01
Copy link
Contributor Author

erincon01 commented Feb 16, 2024 via email

@mivano
Copy link
Owner

mivano commented Feb 16, 2024

I have added token management for accounts that need autentication for
non-default tenants, and for service principal management.
Would you like another PR, or consolidate both PRs?

Oh nice, preferably another PR, please.

@erincon01
Copy link
Contributor Author

Ok, I will wait for your acceptance, and then I will file the new PR. I dont know if can be handle separatelly in git. Thanks.

@@ -210,7 +210,7 @@ private async Task<HttpResponseMessage> ExecuteCallToCostApi(bool includeDebugOu

var currency = row[3].ToString();

var costItem = new CostItem(date, value, valueUsd, currency);
var costItem = new CostItem(date, value, valueUsd, currency, "");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You set the currency to an empty string here. A specific reason for that? This should represent the currency the billing is in.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. Last argument is the Tag. Reason to be empty is because in that method includeTags do not apply

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh sorry, misreading it here.

@mivano mivano merged commit 9761d7b into mivano:main Feb 16, 2024
3 checks passed
Copy link

welcome bot commented Feb 16, 2024

Congrats on merging your first pull request!

@mivano
Copy link
Owner

mivano commented Feb 16, 2024

Looks good, I made some small changes in the tag parsing and cvs formatter. Otherwise the json output will show tags as strings, I converted those to dictionary of string, string. But then I needed a csvformatter to get it in a string again for the CSV output.

So thanks for the PR! I will publish a package asap.

@erincon01
Copy link
Contributor Author

erincon01 commented Feb 16, 2024 via email

@mivano
Copy link
Owner

mivano commented Feb 16, 2024

The package is ready, so have a go with it and see if this suits your needs.

@erincon01
Copy link
Contributor Author

@mivano

the PR I summitted handled the Tags as Json where empty strings were []
now, empty strings are "".
can you review the code?
I see changes from my PR that I cannot follow in git.

@mivano
Copy link
Owner

mivano commented Feb 19, 2024

Is that in the CSV output?

@erincon01
Copy link
Contributor Author

erincon01 commented Feb 19, 2024

in Json:

  {
    "Name": "xxxx",
    "Cost": 1.9521171481699515,
    "Currency": "EUR",
    "CostUsd": 2.107993702451322,
    "Tags": {}
  }

in csv:

Date,Name,Cost,CostUsd,Currency,Tags
2/1/2024,rgyy,1.61312727,1.74193548,EUR,organizationname:sistemas
2/19/2024,rgxx,1.95211715,2.10799370,EUR,

@mivano
Copy link
Owner

mivano commented Feb 19, 2024

Hmm, well the Tags are structured as a dictionary, which translates to an object in JSON, so this is an empty object {} and not an array, hence not using []. For CSV there is no explicit structure, so it needs to translate this dictionary to a flat string.

Are you unable to parse it now at your end?

@erincon01
Copy link
Contributor Author

I see the changes you added. You convert to key-value dictionary here:

        if (includeTags)
        {
            var tagsArray = row[5].EnumerateArray().ToArray();
            
            tags = new Dictionary<string, string>();
            
            foreach (var tagString in tagsArray)
            {
                var parts = tagString.GetString().Split(':');
                if (parts.Length == 2) // Ensure the string is in the format "key:value"
                {
                    var key = parts[0].Trim('"'); // Remove quotes from the key
                    var tagValue = parts[1].Trim('"'); // Remove quotes from the value
                    tags[key] = tagValue;
                }
            }
            currency = row[6].ToString();
        }

I prefered as Json, that is easier to handle in Power Query transformations as Json data type:

        if (includeTags)
        {
            System.Text.Json.JsonElement element = row[5];
            tags = element.GetRawText();
            currency = row[6].ToString();
        }

maybe key-value works fine too in Power Query.
I will try it...

you should change the readme anyway, because I said Tags were Json format.

@mivano
Copy link
Owner

mivano commented Feb 19, 2024

Ah, yeah, I applied some logic I had used before. But they are valid JSON constructions.

image

With your code, I got a string representation in the JSON output (yes, that is valid JSON as well, but would require parsing).

@erincon01
Copy link
Contributor Author

in the json output the format is valid.
however, in the csv, the key:value you've build is not valid a Json.

image

this M instruction fails, because cannot be converted to Json:

= Table.TransformColumns(#"Changed Type",{{"Tags", Json.Document}})

@mivano
Copy link
Owner

mivano commented Feb 19, 2024

Ah got you. Hmm tricky one as it is a CSV output, not a JSON output.

Can you consume the JSON output instead?

Or use a function to convert the string?

let
    ParseStringToRecord = (inputString as text) as record =>
    let
        SplitPairs = Text.Split(inputString, "; "),
        SplitIntoList = List.Transform(SplitPairs, each Text.Split(_, ":")),
        ConvertToListOfRecords = List.Transform(SplitIntoList, each Record.FromList({_[1]}, {_[0]})),
        MergeRecords = List.Accumulate(ConvertToListOfRecords, [], (state, current) => Record.Combine({state, current}))
    in
        MergeRecords
in
    ParseStringToRecord

Table.TransformColumns(Source, {"Tags", each ParseStringToRecord(_), type record})

Or what would be a better format to have it in? I would even think that with a CSV output, I would need a column per tag key. So in your example:

cost, costs, currency, owner, environn, _organisationname_, app

Would that work?

@erincon01
Copy link
Contributor Author

If parsing the text to json in M is easy as you show, it is fine to me.
I thought it was more complicated.
At the end, the "responsability" is in the M side and not in the application.
I will share your M code as reference to the BI guys.
Thanks.

@mivano
Copy link
Owner

mivano commented Feb 19, 2024

I m no power query expert, so this is untested.

I m just hesitant to introduce a JSON object in CSV output, as we already have dedicated JSON output. I have some logic in the WritePricesPerRegion CSV function, which outputs all the columns. So creating additional columns for each tag makes more sense to me. Is that easier to consume?

@erincon01
Copy link
Contributor Author

I would not add new columns from the Tags because tagging follows customer naming conventions, gobernance, etc. etc.
In my case, I prefer the ETL to decide what tags are relevant.

@mivano
Copy link
Owner

mivano commented Feb 19, 2024

They will be dynamic in nature, but it can easily add a lot of additional columns if various different tags are being used. You will get an excellent overview, though, if you have a couple of tags. So, like owner, business unit, cost center, etc, there will be extra columns, and you can easily consume those in Excel and sort/filter on without splitting.

Then it is still up to the ETL what it pulls into the model, but it might be harder to iterate through those dynamic columns to reconstruct the key/value combinations.

Cons and pros; let me know if you can work with the current solution, if not, we come up with something.

@erincon01
Copy link
Contributor Author

erincon01 commented Feb 19, 2024

If you can expand the tags names in columns would be great :)

I'd add:

I'd include the Tag as the last column in case someone wants to extend in Power Query.

BTW, It would be nice to have, not mandatory. I am happy how it is now!
your tool is great as is!

@mivano
Copy link
Owner

mivano commented Feb 19, 2024

Then you get this: #111

Which outputs as this:
image

@erincon01
Copy link
Contributor Author

Yes. Exactly!
I'd recommend using Microsoft tagging rules as a best practice.

In fact, it may be intereseting to the users adding an option --expandColumns "col1,col2,col3" to be specific in what columns to expand. If the columns list is not present, expand with Microsoft tagging said earlier.

but seems a bit complex...

I am not sure if I am suggesting too much, and at the end of the day, nobody uses it :) It may be interesting a pool where people vote to!

@mivano
Copy link
Owner

mivano commented Feb 19, 2024

In my mind, this maps now nicely to CSV. So when you include the tags, you get them as additional columns in CSV output, while in JSON you get them as a dictionary. I have used this way of working before, so it also keeps it consistent.

So I will merge the PR, but do let me know if you can continue to work, as you initiated this PR/discussion for a reason.

@erincon01
Copy link
Contributor Author

it is good how it is now.
I will improve my c# skills and I may ask a PR to add the columns list mapping.
I am an old vb.net guy, but I learn fast :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants